Optional monitors

[yglukhov]

This PR introduces optional monitors feature.
The whole idea is that Object doesn't contain __monitor field anymore.
TypeInfo_Class will hold a monitor offset, if a class has one (by marking class declaration with @monitor attribute), or 0.
Monitor lookup is done with a hash map, if monitorOffset is 0.
The hash map is protected by a primitive RW spin lock.
Monitor finalization will only lookup for monitors, if a monitor was allocated at least once for the type of object being finalized.

If user wants to synchronize over some class, then he will probably want to add a @monitor attribute to it to gain performance bonus (no hash lookup will occur). This attribute will introduce an invisible __monitor field.

DMD counterpart: dlang/dmd#3547
discussion: http://forum.dlang.org/thread/xpliectmvwrwthamquke@forum.dlang.org

[MartinNowak]

Can we also move blank synchronized statements to the hash table?

[yglukhov]

As far as i understood, expression-less synchronized are working in another way. They are actually lowered to entering/exiting a critical section declared static shared in place of synchronized. So there's actually no need for those to be in the table. However, maybe someone who understands this completely should add a comment.

[MartinNowak]

Otherwise I'd favor deprecating synchronized on classes without @monitor.

[MartinNowak]

They are similar in that they both use global mutexes.

[yglukhov]

But there's no need to look em up. They are global and referenced directly. And they are not tied to any object or whatever. Maybe i misunderstood you on this one...

[MartinNowak]

Ah sorry, they should also be looked up, because as it's done right now the compiler has to know the size of the mutex being used. https://github.com/D-Programming-Language/dmd/blob/master/src/target.c#L127

[MartinNowak]

That's a little unrelated though.
Won't a global hash table for non-@monitor classes turn synchronize into an impure statement?

[dnadlinger]

@MartinNowak: Why would it? The fact that the implementation stores part of the object state in global memory doesn't mean that the semantics of synchronized are changed.

[MartinNowak]

Well previously synchronized didn't need any global state, after the change this program would no longer compile.

[MartinNowak]

IMO we should deprecate the old behavior instead of adding a flawed compat layer.

[dnadlinger]

As I mentioned previously, I'd unconditionally support a change towards the explicit-only design if it were not for backwards compatibility, for reasons of simplicity and predictability of performance characteristics. However, I've yet to see a convincing argument against the design proposed here solely on account of correctness. I think we can agree that, if implemented correctly, the change proposed here does not modify observable program semantics. Thus, either the use of synchronized can remain pure (even though it may be backed by an impure implementation internally), or monitors currently are a purity loophole that needs to be fixed.

Now, from a cursory look at the actual diff of the pull request, it seems as though it indeed removes pure from several monitor operations. But if this causes code no longer to compile, this is an implementation problem that can be avoided using suitable casts and not a fundamental design flaw.

[MartinNowak]

I'd unconditionally support a change towards the explicit-only design if it were not for backwards compatibility

That's why I said deprecating not removing, i.e. 12-18 month.

[schveiguy]

Won't a global hash table for non-@monitor classes turn synchronize into an impure statement?

A mutex is by definition shared.

Note that monitors are already special. For example, immutable or const objects can be locked, even though the monitor is modified and is part of the object.

[yglukhov]

Now, from a cursory look at the actual diff of the pull request, it seems as though it indeed removes pure from several monitor operations. But if this causes code no longer to compile, this is an implementation problem that can be avoided using suitable casts and not a fundamental design flaw.

I tried using synchronized statement in a pure function and it worked. Also i see no reason not to. synchronized statement is lowered to function calls _d_monitorenter and _d_monitorexit which are extern(C) and impure.

[MartinNowak]

Let the compiler set this flag instead of doing it lazily.
TypeInfo should be read-only.

[yglukhov]

The idea behind this is to skip monitor finalization for types that never have a monitor. The flag is used in rt_finalize2. Otherwise all types that do not have explicit monitors would suffer a pointless hash-table lookup on finalization.

[MartinNowak]

There is still the read-only issue and flagging a type when you actually want to flag an instance isn't optimal either.

[yglukhov]

You're right, it does look like a hack (or should we call it "implementation specifics"? :). Does it really has to be that read-only? Of course, any other ideas of how to flag an object without adding a field to it are welcome.

[MartinNowak]

You don't flag the object but it's type and yes I'd like to move TypeInfo to the read-only segment.

[MartinNowak]

Ah there is a data-race too, here is the writer.

[MartinNowak]

I'm still uneasy about the whole thing, we're adding a somewhat big machinery to support something we want to get rid of whereas deprecation will took longer but doesn't require an intermediate solution.

[yglukhov]

Well, i can understand you with that. But the "intermediate" part of the solution is not so big. Specifically the part that involves monitor creation/lookup.
What i am afraid of is if we decide to go the "deprecation" way, the question with signals/slots may take ages to discuss. And we can't start deprecation with this question unresolved.

[MartinNowak]

Not seeing it to null when a class has an embedded monitor seems wrong.

[yglukhov]

It's done in _d_monitor_destroy.

[MartinNowak]

What i am afraid of is if we decide to go the "deprecation" way, the question with signals/slots may take ages to discuss. And we can't start deprecation with this question unresolved.

Well std.signal is terribly limited, for a good replacement we need weak ptrs though. And std.signal synchronizes on the object anyhow, so you'll want @monitor too.

[MartinNowak]

And this is the reader of the data-race.

[yglukhov]

If i understand this correctly, the flag is set in the monitor creation code, where finalization can not occur. So it kinda looks like a race condition, but in this case these reads/writes do not overlap. Even if they do, and finalization reads m_flags without hasAllocatedMonitors set, that means that no monitor was allocated for the object being finalized. Please correct me if I'm wrong.

[MartinNowak]

The problem is that finalization can occur while your setting the bit. To guarantee that no word tearing occurs, use atomicWrite!(msync.raw) and atomicRead!(msync.raw). But as I said, we must not mutate the TypeInfo anyhow.

[dnadlinger]

Oh, and there would be no reason to migrate argument-less synchronized blocks to the hash table, as there is no need for indirection there in the first place. It's not like you could instantiate multiple copies of a piece of code at runtime, much unlike for classes, where you can have an unknown number of object instances and need some way to associate a lock to each of them.

[MartinNowak]

Oh, and there would be no reason to migrate argument-less synchronized blocks to the hash table, as there is no need for indirection there in the first place.

I just brought it up because currently the compiler has to know the size of the druntime muted. So I wanted to move this completely to druntime and the easiest solution is to use a hash table from code address to mutex. So it looked quite similar but let's no longer discuss it here.

[yglukhov]

I've been thinking about read-only typeinfos, and it seems like that's not currently possible, because you can synchronize over typeinfo objects, and it requires writing a monitor ptr inside the object. So my approach makes only 1 bit writable, instead of current 64 =).

class A{}
unittest
{
    A a = new A();
    syncronized(typeinfo(a)) { /* smth */ }
}

[MartinNowak]

because you can synchronize over typeinfo objects

Which is the broken behavior you're about to fix.
I still don't see the compelling argument to add all this hash table stuff when we can deprecate monitors for classes without @monitor. The latter is a clean approach which admittedly takes a little longer.

[dnadlinger]

The ability to make typeinfo read-only is indeed a good argument for dropping monitors by default, unless I'm missing some other reason why there needs to be write access. Using a typeinfo instance for synchronization is an anti-pattern anyway…

[yglukhov]

Rebased.

[quickfur]

This is failing to merge again, please rebase (again).

[MartinNowak]

As mentioned here I think we should go for a UDA and properly deprecate the old behavior.
Thanks @yglukhov for your initiative but I'll close this pull for now.

[MartinNowak]

Looks like monitorOffset can never be 0 because that's where the vtable is.
Therefor use 0 to detect unallocated monitors so we can put the class initializer into bss.