Motivation

This bug came about after discussing ongoing parser work with @mgaudet.

The internScope and internScopeCreationData functions are used to register new scope objects with an array of all generated scope objects. The former (internScope) is "old" code - it was the original way in which scope objects were registered within the parser, and it accepts a closure that produces a "real" scope object (i.e. living on the JS heap).

The latter (internScopeCreationData) is used by "new" parser code - matthew's work on moving the parser to not require any interaction with the GC or the GC heap.

Eventually, internScope should go away, and the codepaths that use it deleted, and replaced with usages of internScopeCreationData. This is almost already the case in many parts of the parser - scopes are emitted with a conditional that picks between either creating the scope object immediately within the GC heap, or creating a parser-internal scope object which will be later promoted to a full GC object (at parse complete).

Problem

The problem we've identified is that the internScope and internScopeCreationData functions both accept closures which produce the relevant objects, instead of accepting a pointer to the object itself.

Matthew and I tried to look for reasons it's done this way instead of just having the object created beforehand, there seems to be no compelling reason.

Changing the signatures of internScope and internScopeCreationData to accept object pointers directly instead of thunks, helps simplify the parser.

Notes

Example

Here's an example of where internScope and internScopeCreationData are used (two lines from the same function):

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/js/src/frontend/EmitterScope.cpp#527

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/js/src/frontend/EmitterScope.cpp#538

Note the calls are each in different branches of a conditional - this is an example where we currently implement the "old" and the "new" parser approach.

Uses-list

The following links shows all uses of internScope and internScopeCreationData in the code:

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js8frontend12EmitterScope11internScopeEPNS0_15BytecodeEmitterET_&redirect=false

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js8frontend12EmitterScope23internScopeCreationDataEPNS0_15BytecodeEmitterET_&redirect=false

Notes on internBodyScope and internBodyScopeCreationData

These two functions use internScope and internScopeCreationData internally, but also adjust some global state with the index of the newly added scope. This should be straightforward to fix as well (and its signature will need to be fixed), but I wanted to note it explicitly so that it's not alarming.

Hello, I'd like to work on this bug

Hi Khushal! Thanks for the interest! Feel free to take the bug and work on it. The initial comment should give you a good idea of where to start fixing things. I can review patches and answer questions you may have.

Hello, I think I understand this a bit, although I'm a beginner in contributing so I don't know much, I have few questions

1. How do I test the changes I have done ? are there mochitests for it or something else ?
2. Can you give a little example on how do I do this
   From what I understood, I think I have to change these two functions internScope and internScopeCreationData to accept object pointers instead of the closure (which I think is ScopeCreator createScope?) and the relevant objects which are created refer to Scope* scope
   So The object (which probably is Scope* scope) should be created beforehand in the places where these functions are being used ? (in the places mentioned in Uses-list)
   Please correct me if I have misinterpreted anything

Hi Khushal,

1. Yeah, mochitests, the js shell test, and reference tests should be sufficient to cover testing for this. Do you have access to the try server? This allows you to push your changes to our CI infrastructure to run a gamut of tests against and check. Typically I run the mochitests and call it a day for these things.

2. Your understanding is correct. This bug came about because Matthew and I were looking at this code as part of the larger work of removing JSAtom* usage from the parser. It seemed kind of strange that these functions accepted closures and then invoked them to obtain the object they created, but didn't seem to be doing any real work in between being passed the closure and invoking it - suggesting that this is some extra bit of unnecessary complexity that was added for reasons that aren't relevant anymore.

Ok, as for try server I do not have access to it
So I tried doing changes by creating scope object beforehand and then passing it to 'internScope' and 'internScopeCreationData'
I am getting quite some errors one of them is
member reference type 'js::Scope *' is a pointer; did you mean to use '->' when it runs the line hasEnvironment_ = scope.get().hasEnvironment(); inside the internScopeCreationData function
What should I do?

Can you post (attach, or post via phabricator) a diff of your changes? It would make it easier to advise on what's going wrong.

Yes I attached a phabricator revision

Hey Khushal - will take a look at this and address with phab comments sometime soon. Things are a bit busy with preparation for the all-hands. Leaving needinfo-myself on.

Hi Khushal, taking a look at this patch again. Review comments in a bit.

I sincerely apologize but due to change in my schedule, I won't be able to work on this patch

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

(In reply to Kannan Vijayan [:djvj] from comment #0)

Motivation

This bug came about after discussing ongoing parser work with @mgaudet.

The internScope and internScopeCreationData functions are used to register new scope objects with an array of all generated scope objects. The former (internScope) is "old" code - it was the original way in which scope objects were registered within the parser, and it accepts a closure that produces a "real" scope object (i.e. living on the JS heap).

The latter (internScopeCreationData) is used by "new" parser code - matthew's work on moving the parser to not require any interaction with the GC or the GC heap.

Eventually, internScope should go away, and the codepaths that use it deleted, and replaced with usages of internScopeCreationData. This is almost already the case in many parts of the parser - scopes are emitted with a conditional that picks between either creating the scope object immediately within the GC heap, or creating a parser-internal scope object which will be later promoted to a full GC object (at parse complete).

Problem

The problem we've identified is that the internScope and internScopeCreationData functions both accept closures which produce the relevant objects, instead of accepting a pointer to the object itself.

Matthew and I tried to look for reasons it's done this way instead of just having the object created beforehand, there seems to be no compelling reason.

Changing the signatures of internScope and internScopeCreationData to accept object pointers directly instead of thunks, helps simplify the parser.

Notes

Example

Here's an example of where internScope and internScopeCreationData are used (two lines from the same function):

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/js/src/frontend/EmitterScope.cpp#527

https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/js/src/frontend/EmitterScope.cpp#538

Note the calls are each in different branches of a conditional - this is an example where we currently implement the "old" and the "new" parser approach.

Uses-list

The following links shows all uses of internScope and internScopeCreationData in the code:

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js8frontend12EmitterScope11internScopeEPNS0_15BytecodeEmitterET_&redirect=false

https://searchfox.org/mozilla-central/search?q=symbol:_ZN2js8frontend12EmitterScope23internScopeCreationDataEPNS0_15BytecodeEmitterET_&redirect=false

Notes on internBodyScope and internBodyScopeCreationData

These two functions use internScope and internScopeCreationData internally, but also adjust some global state with the index of the newly added scope. This should be straightforward to fix as well (and its signature will need to be fixed), but I wanted to note it explicitly so that it's not alarming.

I want to work on this bug, I can only find internScopeCreationData in
https://searchfox.org/mozilla-central/search?q=internScopeCreationData&path=
how should I modify the parameters of internScopeCreationData ?

Hi sneha:

internBodyScope has become internBodyScopeCreationData;

In both functions, you'll notice they take as their second argument a C++ Lambda function; but if you look at the implementation of internScopeCreationData, you'll notice that the lambda is -immediately called-; so instead of calling the lambda, we can just replace the creation of a lambda with the body of the lambda, then pass the resulting ScopeIndex that to internScopeCreationData or internBodyScopeCreationData, as appropriate.

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #14)

Hi sneha:

internBodyScope has become internBodyScopeCreationData;

In both functions, you'll notice they take as their second argument a C++ Lambda function; but if you look at the implementation of internScopeCreationData, you'll notice that the lambda is -immediately called-; so instead of calling the lambda, we can just replace the creation of a lambda with the body of the lambda, then pass the resulting ScopeIndex that to internScopeCreationData or internBodyScopeCreationData, as appropriate.

https://paste.mozilla.org/tEsfA8dj
Does it have to be implemented like this?

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #14)

Hi sneha:

internBodyScope has become internBodyScopeCreationData;

In both functions, you'll notice they take as their second argument a C++ Lambda function; but if you look at the implementation of internScopeCreationData, you'll notice that the lambda is -immediately called-; so instead of calling the lambda, we can just replace the creation of a lambda with the body of the lambda, then pass the resulting ScopeIndex that to internScopeCreationData or internBodyScopeCreationData, as appropriate.

sorry I forgot, the earlier link expires in an hour, here is the new one https://paste.mozilla.org/d1dCSU40

https://hg.mozilla.org/mozilla-central/rev/476e81d2fe9b