Get rid of static this for initializing std{in,out,err} #5421

andralex · 2017-05-23T19:41:23Z

Convert awkward globals to functions that return ref File and lazily initialize themselves. This should rid us of a bunch of troubles linked to circular dependencies, betterC etc.

JackStouffer · 2017-05-23T20:02:25Z

std/stdio.d

-    .stderr._p = &stderrImpl;
+    static __gshared File.Impl impl;
+    static __gshared File result;
+    static shared bool lilyWasHere;


lilyWasHere

What?

https://en.wikipedia.org/wiki/Lily_Was_Here

Ahem... not that it's bad to have fun, but I don't think such names should be present in a standard library. We already have a few words in there like "Stuff", that are really non-descript. Or messages worded in terms of "Dunno how to do ", which someone will eventually find difficult to understand. This one is completely internal and not supposed to be looked at all that much, but a person who eventually does have to look is also most likely to go "What???"

Nothing wrong with having a bit of fun, but no problem.

andralex · 2017-05-23T20:24:44Z

std/stdio.d

+        return true;
+    }());
+    return result;
 }


Unclear on why this appears uncovered. It is covered in unittests that assign to stdout.

This unittest just defines a main method, it doesn't actually execute any code...

There's another unittest that assigns to stdout.

WalterBright · 2017-05-23T22:54:27Z

This seems more complex than necessary. Just make stdin a pointer to a function that returns a ref. Statically initialize it to a function that initializes stdin, and rewrites the function pointer to point to a function that returns a ref without checking initialization.

andralex · 2017-05-23T23:00:11Z

@WalterBright that won't work for e.g. functions that take a File and get passed stdout.

andralex · 2017-05-23T23:21:54Z

Updated

CyberShadow · 2017-05-24T12:33:45Z

I'm guessing the performance implication of this is likely negligible? Tot that it really matters, but I wonder if an indirect function call is going to be less expensive than an atomic load...

andralex · 2017-05-24T14:19:01Z

Been thinking of the pointer to function approach. In summary: it can be made to work and in all likelihood it is marginally more efficient, but it's more complicated than the code in this PR.

An arbitrary number of threads may enter the "initialize" pointer to function simultaneously. So inside the function that does the initialization code must exist that does essentially what initOnce does right now. Then the initialization code changes the pointer to the "steady state" function.

Then, for each call in the steady state, we have an atomic load of the pointer to function and an indirect call. On x86 the atomic load is free.

radcapricorn · 2017-05-24T19:15:37Z

std/stdio.d

-        result._p = &impl;
-        return true;
-    }());
+    if (!initialized)


Seems redundant, initOnce does effectively the same thing internally. If this is to help with inlining the atomicLoad, aren't we better off fixing the compiler instead of doing this "triple checked singleton"?

Scratch that, it's still better for platforms where atomicLoad won't be cheap.

dnadlinger · 2017-05-24T19:32:31Z

std/stdio.d

 }

-extern(C) void std_stdio_static_this()
+// Undocummented but public because std* handles are aliasing it


mmmmmmmh, typos.

dnadlinger · 2017-05-24T19:36:51Z

std/stdio.d

+    static shared bool initialized;
+    if (!initialized)
+    {
+        // Use double-checking because initOnce is inefficient.


Could you explain how adding a third thread-global flag that is set in exactly the same way is more efficient? (I might be missing something obvious.)

Also, depending on what the semantics for shared in D are, using a plain if might strictly speaking not be well-defined – it would be a data race in C++ and hence undefined behaviour. This is why we need to define a memory model! (I don't think LLVM or GCC LTO would currently cause problems, though.)

It's not third, it's second, the same it was before.

I tripped over that at first too. initOnce() internally uses the double-checking pattern with atomicLoad, which may or may not be more expensive than a plain load depending on platform. But 'true' can never be observed on this new extra check before initOnce() has completed, so in effect this simply short-circuits a potentially more expensive load. It may come at a cost of a few extra calls to initOnce() though, depending on how memory works on a particular platform.
If it was allowed for initialized to flip from true to false during runtime, that new if would indeed have been a mistake.

IIRC, TDPL initially proposed that reads and writes from/to shared should be both atomic and fenced. That last part is a very heavy restriction though, and, among other things, shows exactly why we need a DIP to make shared actually work.

The reason why it was found "slow" on x86 is due to dmd's poor inlining.

The problem I see here is that initialized is perfectly race-free. But the setting of the handle is a side effect. So is it possible one thread reads true while the handle has not yet been set?

I'm somewhat lost with the way this is done. You are protecting a boolean with a complicated set of calls using atomics and a mutex, and I don't fully understand why they are all there. The result value seems unprotected, as it's never loaded or stored atomically, whereas the booleans explicitly are inside initOnce.

I tripped over that at first too.

So far, I don't think I tripped over anything. ;)

initOnce() internally uses the double-checking pattern with atomicLoad, which may or may not be more expensive than a plain load depending on platform. But 'true' can never be observed on this new extra check before initOnce() has completed, so in effect this simply short-circuits a potentially more expensive load.

What makes you think that if this optimization is valid, initOnce can't employ it all the same? It's specifically designed for this use case.

initOnce can't optimize it. It has to take the (potentially) fenced approach in order to make sure it does indeed store the initializer only once, and that may be slower on non-x86. It may be called multiple times, but only once should it actually store. But there is no reason to call it at all, and thus incur a penalty, if true is already observed.

@schveiguy, initializer is stored under a mutex, which should imply a fence. Sigh... "should". @klickverbot is right, we need a formalized model.

I'm somewhat lost with the way this is done. You are protecting a boolean with a complicated set of calls using atomics and a mutex, and I don't fully understand why they are all there. The result value seems unprotected, as it's never loaded or stored atomically, whereas the booleans explicitly are inside initOnce.

initOnce employs a double-checked locking pattern. Which is check - lock - check again. Actual store, both of the initializer and the flag, is performed only under a lock, and only if the second check agrees with the first one. This prevents second thread from writing an initializer a second time, if it blocked on a mutex after the first check but before another thread has finished storing. Storing itself does not have to be atomic, so long as the observability upon releasing the lock is guaranteed.
Why this was slow in Andrei's measurements was because dmd does a poor job inlining the first check. So he added an unencumbered read to circumvent that.

Taking a look at the code again, it seems I am wrong. It still stores the value (in this case the external boolean initialized) non-atomically. So I think it's fine the way this is done (albeit very convoluted). initOnce needs an overload that doesn't assume the initialization is done by assignment (as in this case, it cannot be).

@klickverbot I think the optimization is valid, because initialized will never go from true to false. A similar technique could be employed inside initOnce, but I think it still doesn't help because of the way initialization is assumed.

The short version:

But there is no reason to call it at all, and thus incur a penalty, if true is already observed.

Careful with the word observed. Non-atomic reads – for the sake of the argument, let's use the C++11 memory model and consider the read from initialized relaxed instead of plainly being a data race – don't come with any sequential consistency guarantees, and so the execution order constraints you can infer from not taking the if are minimal.

The longer, hopefully more enlightening version in very much of a Socratic spirit (not that there would much of a basis for discussion if we don't assume, say, C++11 semantics for D):

initOnce can't optimize it.

What about literally just moving the if inside initOnce?

It has to take the (potentially) fenced approach in order to make sure it does indeed store the initializer only once

That's already guaranteed by the check that makes the "doubled" part of the double-checked locking pattern, isn't it?

It's not about storing the initializer atomically at all. It's about observing both it's value and the flag value after they've been stored.
It is exactly the same technique inside initOnce already, only initOnce can't count on plain load, it has to do an acquire (which, on some platforms, requires a memory fence).

As for overloads, it also needs a @nogc and nothrow one. The latter is a bug with lazy, for the former we'd need to allocate the mutex without the GC. I'd propose ditching the lazy parameter in favor of template callable, but I fear that ship has sailed.

Careful with the word observed. Non-atomic reads – for the sake of the argument, let's use the C++11 memory model and consider the read from initialized relaxed instead of plainly being a data race – don't come with any sequential consistency guarantees, and so the execution order constraints you can infer from not taking the if are minimal.

Reads from shared integrals should be atomic, according to TDPL. They should not interleave with writes. I.e. there should not be a data race there.

The longer, hopefully more enlightening version in very much of a Socratic spirit (not that there would much of a basis for discussion if we don't assume, say, C++11 semantics for D):

initOnce can't optimize it.

What about literally just moving the if inside initOnce?

We can't. It's of arbitrary type, might not even be convertible to bool, or true might not actually be the expected value.

It has to take the (potentially) fenced approach in order to make sure it does indeed store the initializer only once

That's already guaranteed by the check that makes the "doubled" part of the double-checked locking pattern, isn't it?

You're looking at the wrong boolean :) initOnce has its own flag, which it uses for its' double-checked locking. Variable initialized just happens to be a boolean.

So, what happens is:

thread reads (atomically, but perhaps out of order) initialized, not using any fences.

it may read false incorrectly, and consequently call initOnce. But it will never read true incorrectly.

if it read false and called initOnce, initOnce has to make sure it doesn't read it's own flag out of order w.r.t. writes.

So Andrei's check looks redundant w.r.t. to initOnce, but it isn't, as it's guaranteed cheap.

andralex · 2017-05-24T22:46:43Z

Thanks for all reviews. May I suggest a means to improve the efficiency of reviews.

The PR is an engineering solution in the sense that it uses the tools available (in this case: initOnce and shared bool) to achieve a purpose (in this case lazy initialization of the stdc file handles). A way to look at this is:

Is this a valid engineering solution, i.e. does it use the available tools appropriately to achieve the need? If not, suggest better ways to do so.
Could the tools available be better? In this case, a runOnce function in the style of https://linux.die.net/man/3/pthread_once would make things clearer and simpler, assuming it can be implemented efficiently.

What happens sometimes is that discussion on the first topic (appropriate for the PR) get into the second. At best, we would enhance collaboration by helping the PR creator instead of assigning him/her the second task in addition to the first one. Right now we are in the position where a couple of collective hours have already been spent in debating what would be correct and what wouldn't. The same time could be spent infinitely more productively by simply writing a better tool (e.g. runOnce or something better) and proposing it in the form of a PR for the standard library.

So, @klickverbot you clearly have the know-how and the enthusiasm in the topic. Could you please implement runOnce or whichever primitive you think is best?

andralex · 2017-05-24T22:49:43Z

(I'll note that @radcapricorn would be just as indicated for the task but if I asked "could @klickverbot or @radcapricorn ..." each would assume the other would do it and there'd be no progress.)

radcapricorn · 2017-05-24T22:53:27Z

Thanks :) I would've asked for another candidate in any case, I already have a few PRs which I want to see through into Phobos or rejected first.

andralex · 2017-05-24T23:47:48Z

I committed an alternative implementation that does not use initOnce. LMK if that's preferable.

dnadlinger · 2017-05-24T23:52:27Z

May I suggest a means to improve the efficiency of reviews.

I'm all for that – this is actually my main frustration with contributing to the core language.

Is this a valid engineering solution

It isn't, it is racy. (See below for assumptions.)

So, @klickverbot you clearly have the know-how and the enthusiasm in the topic. Could you please implement runOnce or whichever primitive you think is best?

It's already there (initOnce). The only problem is DMD's inliner, which you can work around by manually inlining the parts as necessary if you so desire. I'm not interested in doing that myself (and would mildly object to doing so, although I recognise that you might differ in your analysis of the engineering tradeoffs involved).

The same time could be spent infinitely more productively by simply writing a better tool (e.g. runOnce or something better) and proposing it in the form of a PR for the standard library.

Disagree. I'd claim my time is much better spent reminding people of the finer points in topics I'm knowledgeable about than doing all the work myself, so that they'll get it right themselves next time around. In this particular case, the tool you need already exists and works, unless one breaks it by trying to be clever. ;)

To meaningfully discuss the correctness of multi-threaded programs, we need to have execution semantics that allow for multiple threads of execution. D currently has no formal memory model, but given that the good old "hope that the compiler doesn't transform the code too much" approach works just fine with DMD (helped by the almost-sequentially-consistent guarantees x86 provides) and both GDC/LDC use C++ semantics, let's just consider the C++11 memory model. We really need to formalise this for D to have a claim to be a "no space below" systems programming language, but that's a topic for another day.

Furthermore, let's not worry about the fact that data races are undefined behaviour in C++11 (and a compiler detecting this statically could apply arbitrary transformations), and just concentrate on the fact whether unintended outcomes can be observed by considering all accesses to initialized and any other shared state set by the initializer to be relaxed atomic load/stores.

The problem with the proposed change is that loading true from initialized on thread A is not enough to introduce a happens-before edge from the stores to the FILE variables on another thread B that ran the initializers to whatever later uses on thread A there might be. Non-atomic accesses to different memory locations are simply not ordered with respect to each other at all. You (@andralex) had a paper on double-checked locking, right? This would be exactly the reason why you need some sort of memory barrier/acquire-release atomics to make double-checked locking work.

dnadlinger · 2017-05-24T23:53:41Z

I committed an alternative implementation that does not use initOnce. LMK if that's preferable.

Just saw it, will have a look. As far as I can see, initOnce would have been fine too, though.

radcapricorn · 2017-05-24T23:56:17Z

std/stdio.d

+            {
+                impl.handle = handle;
+                result._p = &impl;
+                atomicOp!"+="(initialized, uint.max / 2);


This one will not be correct even on x86, it would require a memory fence. I'm for the initOnce version.

Are you saying there is a bug in atomicOp?

No. impl.handle, result._p and initialized are different memory locations. A further load from result._p may get reordered w.r.t. store to impl.handle.

@radcapricorn as @klickverbot said, that's a topic for another day. For now let's simplify our lives by using the same assumptions that initOnce makes: https://github.com/dlang/phobos/blob/master/std/concurrency.d#L2408

Please try running this code either with ldc or dmd.

And compare against this one.

andralex · 2017-05-25T00:04:14Z

@klickverbot The fly in the ointment is __gshared, which indeed is poorly defined. The code (whether using initOnce or atomic integrals) assumes that stores to __gshared variables cannot be moved across reads or writes of shared data. This is similar to the way Java defines the interaction between volatile and non-volatile data.

dnadlinger · 2017-05-25T00:21:29Z

The code (whether using initOnce or atomic integrals) assumes that stores to __gshared variables cannot be moved across reads or writes of shared data.

LDC as well as, from what I understand, GDC currently treat shared only as a marker for the type system. Unless you use the core.atomic primitives, you'll just get the regular semantics (same as with the explicit atomics in C++11).

This is similar to the way Java defines the interaction between volatile and non-volatile data.

I haven't really looked at the Java situation in any detail, but I believe volatile is essentially equivalent to requiring sequential consistency on every operation in the C++ model.

radcapricorn · 2017-05-25T00:22:00Z

@klickverbot

D currently has no formal memory model, but given that the good old "hope that the compiler doesn't transform the code too much" approach works just fine with DMD...

This one should not apply. According to TDPL, shared implies a compiler barrier, so the compiler should not reorder anything past shared read or write instructions (I see @andralex confirms that). If any of the compilers do that today, that's a bug.

The problem with the proposed change is that loading true from initialized on thread A is not enough to introduce a happens-before edge from the stores to the FILE variables on another thread B that ran the initializers to whatever later uses on thread A there might be. Non-atomic accesses to different memory locations are simply not ordered with respect to each other at all.

Yes it is in the initOnce version, because both stores are protected by a memory barrier (incurred by the mutex). It can load false after FILE was stored, yes. It can never load true before that.

andralex · 2017-05-25T00:27:48Z

@klickverbot could you please please kindly please write five lines of code that clarify what you say? Essentially could you please paste the code as you think LDC would find it correct? Thanks!

jmdavis · 2017-05-25T00:30:29Z

According to TDPL, shared implies a compiler barrier, so the compiler should not reorder anything past shared read or write instructions (I see @andralex confirms that).

Unless something has changed very recently, in spite of what TDPL says, shared does not currently imply a barrier, and it's been debated as to whether it should (because of the performance hit that memory barriers incur). It's one of those aspects of shared that really needs to be fully defined but hasn't been. So, even if we all agreed that it should imply a memory barrier, it's not what the compiler does, and relying on it would be error-prone.

radcapricorn · 2017-05-25T00:39:15Z

@jmdavis compiler barrier, not memory. The part about shared and memory barriers in the book was fuzzy at best. And IMHO, there should be no such requirement, as each case is unique and requires a manual approach.

dnadlinger · 2017-05-25T00:39:18Z

@radcapricorn: We might want to take this elsewhere to keep the PR on-topic. It boils down to compiler barriers not being sufficient to guarantee sequential consistency of the memory effects. Of course, it will still be fine on x86 as MOVs for loads/stores are implicitly acquire/release.

@andralex:

@klickverbot could you please please kindly please write five lines of code that clarify what you say? Essentially could you please paste the code as you think LDC would find it correct? Thanks!

initOnce is fine, as I said. Zero lines of code necessary.

shared doesn't imply anything without using atomic operations on LDC (for general lack of a relevant language specification to follow). That doesn't impact initOnce, as it is using explicit atomics and these are interpreted like their C++ equivalents.

I didn't have a chance to properly think about your alternative code yet, but it seems correct at first glance.

radcapricorn · 2017-05-27T19:21:40Z

@andralex done. Yeah, that old address should be dead.

dnadlinger

Should be okay now if we are fine with the added complexity.

andralex · 2017-06-01T18:11:05Z

thx!

MartinNowak

Please use initOnce for that!

MartinNowak · 2017-06-01T19:42:14Z

This function was added about a year ago to simplify lazy initialization, no need to roll your own spinlock either @andralex.

andralex · 2017-06-01T19:43:36Z

@MartinNowak the initial version used initOnce indeed. It marked a performance regression for using std* handles.

MartinNowak · 2017-06-06T22:06:06Z

@MartinNowak the initial version used initOnce indeed. It marked a performance regression for using std* handles.

I'll have a look at http://forum.dlang.org/post/og4d2d$fkc$1@digitalmars.com, doesn't seem like much investigation happened.

Meanwhile this PR broke unit-threaded Issue 17472 – [Reg 2.075] typeof(stdin) is no longer a File.

dnadlinger · 2017-06-06T23:36:18Z

@MartinNowak: Agreed, as mentioned above I don't think the inline spinlock thing is worth it (my green checkmark thing concerned the actual implementation, which was broken before).

Not too sure if/how we want to tackle the type issue. As far as I can see, we'd need to keep the static constructor (but maybe could offer an explicit initialization function for runtime-less builds, if this is where @andralex et al. are aiming.

andralex · 2017-06-06T23:44:48Z

Generally we should offer implementations the freedom to swap properties for variables. Granted, that's not present in the specification so yes this PR is liable to break code that queries the type of stdin etc. But we need to make progress on larger issues and not all breakages are equal. This change stays.

MartinNowak · 2017-06-07T00:58:00Z

std/stdio.d

-    .stderr._p = &stderrImpl;
-}
+    static __gshared File.Impl impl;
+    static __gshared File result;


The static is redundant here.

Will edit the file in place.

MartinNowak · 2017-06-09T12:26:24Z

Generally we should offer implementations the freedom to swap properties for variables.

Only you didn't make it a @property, this being the case where it matters.

struct S {}

S foo() { return S(); }
@property S bar() { return S(); }

pragma(msg, typeof(foo));
pragma(msg, typeof(bar));

MartinNowak · 2017-06-09T12:28:33Z

@MartinNowak: Agreed, as mentioned above I don't think the inline spinlock thing is worth it (my green checkmark thing concerned the actual implementation, which was broken before).

Performance of initOnce isn't that well because a common Mutex is passed as default argument, but this one is also created lazily. Combined with dmd's incapable inliner, this makes 2 function calls, and 2 atomic loads (in non-inlineable asm block functions).

andralex · 2017-06-09T12:45:22Z

@MartinNowak thanks for the PR. @mdparker interested in a blog on "The One Use Case of @property"?

wilzbach · 2017-06-09T12:51:10Z

@mdparker interested in a blog on "The One Use Case of @Property"?

@mdparker if you follow up on this, accessors and this NG post might be helpful.

mdparker · 2017-06-09T12:56:51Z

Yeah, I'll add it to my list for after my @nogc post.

CyberShadow · 2017-06-09T14:55:26Z

interested in a blog on "The One Use Case of @Property"?

Are you sure this is a good idea? I mean... https://wiki.dlang.org/Language_issues#Properties

andralex · 2017-06-09T15:07:28Z

I think @property has gotten to a stable fixed point; any improvements we may add to it will allow more code to compile without breaking what it serves for right now.

John-Colvin · 2017-06-26T14:07:13Z

Perhaps I missed this in the discussion about, but this P.R. breaks code that takes the address of std{in,out,err}, i.e. File* a = &stdin;.

John-Colvin · 2017-07-12T11:02:51Z

Is there a reason why makeGlobal isn't @trusted? Seems mad that std{err,out}.writeln("some string literal") can't happen in @safe code.

andralex · 2017-07-16T01:48:24Z

@John-Colvin missed that. Could you please create a short PR? Thanks!

wilzbach · 2017-07-16T11:07:45Z

Is there a reason why makeGlobal isn't @trusted? Seems mad that std{err,out}.writeln("some string literal") can't happen in @safe code.

In case someone is wondering whether that's a regression - it's luckily not.
Before this wasn't possible due to Issue 15768

import std.stdio;
void main() @safe {
    stderr.writeln("Hello World");
}

JackStouffer · 2017-07-16T15:52:21Z

Yes, and until that issue is addressed I'm strongly against marking that function trusted. I'll try to take a look at that issue again today; maybe it's easier to fix now, but it may still turn out that File needs major restructuring for shared.

Get rid of static this for initializing std{in,out,err}

099fa37

andralex mentioned this pull request May 23, 2017

file.d: add overflow checks #4712

Closed

JackStouffer reviewed May 23, 2017

View reviewed changes

andralex commented May 23, 2017

View reviewed changes

Make it less fun

a854092

radcapricorn reviewed May 24, 2017

View reviewed changes

dnadlinger reviewed May 24, 2017

View reviewed changes

initOnce is slow, use double checking

38ebccd

Alternative implementation that doesn't use initOnce

ee31837

andralex force-pushed the no-more-durn-stdiobase branch from 6ef3b5a to ee31837 Compare May 24, 2017 23:45

radcapricorn reviewed May 24, 2017

View reviewed changes

dnadlinger approved these changes May 27, 2017

View reviewed changes

andralex added the Merge:auto-merge label Jun 1, 2017

dlang-bot merged commit 76d3a41 into dlang:master Jun 1, 2017

MartinNowak reviewed Jun 1, 2017

View reviewed changes

andralex deleted the no-more-durn-stdiobase branch June 1, 2017 19:43

MartinNowak reviewed Jun 7, 2017

View reviewed changes

CyberShadow mentioned this pull request Jun 7, 2017

[trivial] Fix bracing style in std.traits #5449

Merged

wilzbach mentioned this pull request Jun 9, 2017

No completion with std.stdio stdin dlang-community/DCD#305

Closed

MartinNowak mentioned this pull request Jun 9, 2017

fix Issue 17472 - typeof(stdin) is no longer a File #5454

Merged

joakim-noah mentioned this pull request Aug 1, 2017

Merge 2.075.1 front-end and stdlibs ldc-developers/ldc#2252

Merged

wilzbach mentioned this pull request Jan 28, 2018

Avoid the need of calling initDMD() in library code dlang/dmd#7800

Closed

Uh oh!

Get rid of static this for initializing std{in,out,err} #5421

Get rid of static this for initializing std{in,out,err} #5421

Uh oh!

Conversation

andralex commented May 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright commented May 23, 2017

Uh oh!

andralex commented May 23, 2017

Uh oh!

andralex commented May 23, 2017

Uh oh!

CyberShadow commented May 24, 2017

Uh oh!

andralex commented May 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radcapricorn May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

radcapricorn May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnadlinger May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andralex commented May 24, 2017

Uh oh!

andralex commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

radcapricorn commented May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andralex commented May 24, 2017

Uh oh!

dnadlinger commented May 24, 2017

Uh oh!

dnadlinger commented May 24, 2017

dnadlinger May 24, 2017 •

edited

Loading

radcapricorn May 24, 2017 •

edited

Loading

radcapricorn May 24, 2017 •

edited

Loading

dnadlinger May 24, 2017 •

edited

Loading

andralex commented May 24, 2017 •

edited

Loading

radcapricorn commented May 24, 2017 •

edited

Loading

radcapricorn May 25, 2017 •

edited

Loading

andralex May 25, 2017 •

edited

Loading

radcapricorn May 25, 2017 •

edited

Loading

dnadlinger commented May 25, 2017 •

edited

Loading