The global lexical is stored in a GlobalObject reserved slot but that makes it hard to read from JIT code. The current code is wrong because GlobalObject::offsetOfLexicalEnvironmentSlot doesn't account for the global having a private slot (XPConnect globals!).

We should move it to the Realm. That also eliminates a dereference, unboxing, and isFixedSlot lookup in C++.

Longer term we should consider moving all these special slots from GlobalObject to Realm.

This way it's easier and faster to access, especially from JIT code.

The old code in GlobalObject::offsetOfLexicalEnvironmentSlot was wrong because
it didn't account for the global having a private slot (XPConnect globals!).

Depends on D26083

Hey Jon, I'm not sure whether using HeapPtr (instead of ReadBarriered) for Realm::lexicalEnvironment_ is okay?

I think it is because we trace the lexical environment whenever we trace the global so it's very similar to the old code reading it out of a GlobalObject slot without a read barrier?

(In reply to Jan de Mooij [:jandem] from comment #3)

I think it is because we trace the lexical environment whenever we trace the global so it's very similar to the old code reading it out of a GlobalObject slot without a read barrier?

Hm that probably isn't true because getting the global could have triggered the barrier.

The problem is that cx->global() is smart enough not to trigger the read barrier, but it would be unfortunate having to copy that mechanism for the lexical environment and other things in the future.

(In reply to Jan de Mooij [:jandem] from comment #3)

(Posting the comment I wrote before I saw your last comment)

At the moment we have the situation where we ensure globals from all on-stack realms get marked. With this change that would also mark the lexical environment. So I think we don't need a barrier as long is it's a realm we have entered. Can we assert that?

Is the lexical environment still logically part of the global but stored externally or is it part of the realm now? If it's the former HeapPtr is fine but we need a comment about why it's OK to skip the global read barrier when reading the lexical environment. If it's the latter it would be better as ReadBarriered and traced from Realm::traceRoots() as we do for the global (also with unbarrieredGet() to read). In general I prefer ReadBarriered for weak pointers for documentation even if we always skip the barrier.

(In reply to Jan de Mooij [:jandem] from comment #4)

it would be unfortunate having to copy that mechanism for the lexical environment and other things in the future.

Hmm, I see what you mean, it's not great.

(In reply to Jon Coppeard (:jonco) from comment #5)

So I think we don't need a barrier as long is it's a realm we have entered. Can we assert that?

Not easily, unfortunately, because Realm::enterRealmDepthIgnoringJit_ does not account for realm entering in JIT code.

I'll think about it more. Maybe I should use a ReadBarriered and use unsafeGet in JSContext::globalLexicalEnvironment (to be added) and GlobalObject::lexicalEnvironment.

Thanks!

@Jon: I updated the patch. This version is much simpler and I think still good enough.

https://hg.mozilla.org/mozilla-central/rev/1eaf2a656e3f
https://hg.mozilla.org/mozilla-central/rev/1e3fc1d21f8a