Bugzilla

Nicolas B. Pierron [:nbp]

Comment 2

•

4 years ago

Sure, I could mentor this.

Mentor: iireland

Flags: needinfo?(iireland)

Flags: needinfo?(andrebargull)

Updated

•

4 years ago

Whiteboard: [lang=c++]

Assignee

Comment 3

•

4 years ago

Hi, I would like to work on this bug. Can you please guide me a little bit...
Thanks

Flags: needinfo?(iireland)

Comment 4

•

4 years ago

Hi there Shivam here, would like to work on this bug.
I have contributed in past, looking to submit a patch .

Comment 5

•

4 years ago

Hi there Shivam here, would like to work on this bug.
I have contributed in past, looking to submit a patch .

Comment 6

•

4 years ago

sorry for the double message.

Comment 7

•

4 years ago

Sanchit: Sure!

The macro-assembler (masm) is our way of generating code. It has many helper functions that perform some operation. For example, branchTestBigInt (which was added by the patch linked above) will check to see if a JS::Value has the BigInt tag, and will jump to a label based on the condition (either Equal or NotEqual).

In a number of places, we want to check whether a value is in the nursery (the garbage collector's set of recently allocated objects). Only Objects, Strings, and BigInts can be in the nursery, so the first step is to see whether the value is one of those types. The goal of this bug is to tidy up that check. Instead of checking for each of the types every time, it would be better if there was a "branchTestNurseryAllocatable" function.

This would make it easier to update the code if we start allocating a new kind of value in the nursery. It might also make the code faster, if we can avoid doing redundant work. (Above, anba mentions not having to split the tag multiple times, but it looks to me at first glance like we're already managing that. I might be missing something that will be clearer once you get deeper into the code, though.)

Have you contributed to SpiderMonkey before? The first step is to get a build of SpiderMonkey working and to get familiar with the code. SearchFox is an incredibly useful tool for looking around in the code. For example, here are all the places we currently use branchTestBigInt.

Hopefully that should get you started! Need-info me if you get stuck.

Shivam: Sanchit asked about this bug just before you, so we will give them the first shot at it.

Flags: needinfo?(iireland)

Assignee

Comment 8

•

4 years ago

(In reply to Iain Ireland [:iain] from comment #7)

Sanchit: Sure!

The macro-assembler (masm) is our way of generating code. It has many helper functions that perform some operation. For example, branchTestBigInt (which was added by the patch linked above) will check to see if a JS::Value has the BigInt tag, and will jump to a label based on the condition (either Equal or NotEqual).

In a number of places, we want to check whether a value is in the nursery (the garbage collector's set of recently allocated objects). Only Objects, Strings, and BigInts can be in the nursery, so the first step is to see whether the value is one of those types. The goal of this bug is to tidy up that check. Instead of checking for each of the types every time, it would be better if there was a "branchTestNurseryAllocatable" function.

This would make it easier to update the code if we start allocating a new kind of value in the nursery. It might also make the code faster, if we can avoid doing redundant work. (Above, anba mentions not having to split the tag multiple times, but it looks to me at first glance like we're already managing that. I might be missing something that will be clearer once you get deeper into the code, though.)

Have you contributed to SpiderMonkey before? The first step is to get a build of SpiderMonkey working and to get familiar with the code. SearchFox is an incredibly useful tool for looking around in the code. For example, here are all the places we currently use branchTestBigInt.

Hopefully that should get you started! Need-info me if you get stuck.

Shivam: Sanchit asked about this bug just before you, so we will give them the first shot at it.

Thanks for taking out time to explain everything in detail.
What I figured out from https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm-inl.h#1656 is that for each type (Objects, Strings, BigInts) there are 4 overloaded functions which do the checking. So am I supposed to create 4 overloaded functions ( all with name branchTestNurseryAllocableTag ) and in each of them call the functions corresponding to the parameters given for each type to do the checking?
Please let me know if I'm going in wrong direction.
Thank You

Flags: needinfo?(iireland)

Comment 9

•

4 years ago

There is another similar issue can I work on that ?

Comment 10

•

4 years ago

What I figured out from https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm-inl.h#1656 is that for each type (Objects, Strings, BigInts) there are 4 overloaded functions which do the checking. So am I supposed to create 4 overloaded functions ( all with name branchTestNurseryAllocableTag ) and in each of them call the functions corresponding to the parameters given for each type to do the checking?

One of those overloads is doing something slightly different than the others. It might help if I explain our value representation quickly.

A JS::Value represents a value that can be passed around in javascript code. This can be a number (Double or Int32), an Object, a String, a BigInt, or a variety of other cases. We use some clever tricks to store everything inside an 8-byte double precision floating point value, using NaNs to represent all JS values that are not doubles. All you really need to understand is that the first N bits of each 8-byte Value (where N depends on whether we are 32-bit or 64-bit) are a tag, that indicates how to interpret the remaining bits. For example, if a value is tagged as a string, bigint, object, or symbol, the remaining bits are a pointer to the actual value.

Three of the overloads work on Values. The ValueOperand overload works on a Value that is in a register. The Address and BaseIndex overloads work on Values in memory. (Address is one base register plus a fixed offset; BaseIndex is a base register plus an index register plus a displacement.) The last overload ("Register tag") works on a tag value that has already been extracted from a Value. The advantage is that we don't have to split the tag each time if we are going to compare it against several different options.

All of that is to say: yes, you should probably have four overloads. I think branchTestNurseryAllocatable is likely a better name. The version that takes a tag will just call the tag-specialized versions of branchTestString/etc. The other versions should split the tag, then call the tag-specialized versions.

The next step is to find places that currently call the sequence of branchTests and update them to use the new helper.

Good luck!

Flags: needinfo?(iireland)

Assignee

Comment 11

•

4 years ago

(In reply to Iain Ireland [:iain] from comment #10)

What I figured out from https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm-inl.h#1656 is that for each type (Objects, Strings, BigInts) there are 4 overloaded functions which do the checking. So am I supposed to create 4 overloaded functions ( all with name branchTestNurseryAllocableTag ) and in each of them call the functions corresponding to the parameters given for each type to do the checking?

One of those overloads is doing something slightly different than the others. It might help if I explain our value representation quickly.

A JS::Value represents a value that can be passed around in javascript code. This can be a number (Double or Int32), an Object, a String, a BigInt, or a variety of other cases. We use some clever tricks to store everything inside an 8-byte double precision floating point value, using NaNs to represent all JS values that are not doubles. All you really need to understand is that the first N bits of each 8-byte Value (where N depends on whether we are 32-bit or 64-bit) are a tag, that indicates how to interpret the remaining bits. For example, if a value is tagged as a string, bigint, object, or symbol, the remaining bits are a pointer to the actual value.

Three of the overloads work on Values. The ValueOperand overload works on a Value that is in a register. The Address and BaseIndex overloads work on Values in memory. (Address is one base register plus a fixed offset; BaseIndex is a base register plus an index register plus a displacement.) The last overload ("Register tag") works on a tag value that has already been extracted from a Value. The advantage is that we don't have to split the tag each time if we are going to compare it against several different options.

All of that is to say: yes, you should probably have four overloads. I think branchTestNurseryAllocatable is likely a better name. The version that takes a tag will just call the tag-specialized versions of branchTestString/etc. The other versions should split the tag, then call the tag-specialized versions.

The next step is to find places that currently call the sequence of branchTests and update them to use the new helper.

Good luck!

Thanks a lot for your guidance! I'll soon submit a patch for this bug.

Comment 12

•

4 years ago

Shivam: If you are looking for something to work on, the children of bug 1478034 might be a good place to start. For example, take a look at bug 1587575.

Assignee

Comment 13

•

4 years ago

(In reply to Iain Ireland [:iain] from comment #10)

What I figured out from https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm-inl.h#1656 is that for each type (Objects, Strings, BigInts) there are 4 overloaded functions which do the checking. So am I supposed to create 4 overloaded functions ( all with name branchTestNurseryAllocableTag ) and in each of them call the functions corresponding to the parameters given for each type to do the checking?

One of those overloads is doing something slightly different than the others. It might help if I explain our value representation quickly.

A JS::Value represents a value that can be passed around in javascript code. This can be a number (Double or Int32), an Object, a String, a BigInt, or a variety of other cases. We use some clever tricks to store everything inside an 8-byte double precision floating point value, using NaNs to represent all JS values that are not doubles. All you really need to understand is that the first N bits of each 8-byte Value (where N depends on whether we are 32-bit or 64-bit) are a tag, that indicates how to interpret the remaining bits. For example, if a value is tagged as a string, bigint, object, or symbol, the remaining bits are a pointer to the actual value.

Three of the overloads work on Values. The ValueOperand overload works on a Value that is in a register. The Address and BaseIndex overloads work on Values in memory. (Address is one base register plus a fixed offset; BaseIndex is a base register plus an index register plus a displacement.) The last overload ("Register tag") works on a tag value that has already been extracted from a Value. The advantage is that we don't have to split the tag each time if we are going to compare it against several different options.

All of that is to say: yes, you should probably have four overloads. I think branchTestNurseryAllocatable is likely a better name. The version that takes a tag will just call the tag-specialized versions of branchTestString/etc. The other versions should split the tag, then call the tag-specialized versions.

The next step is to find places that currently call the sequence of branchTests and update them to use the new helper.

Good luck!

I added all the four overloads but I'm unable to find any place that call these sequence of branchTests and are replacable with the new helper.
The only file in which call to branchTests are made in sequence is https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp (line 4572 and 4591) but in that too the parameters for branchTestBigInt is not same as branchTestString or branchTestObject.
Am I missing something or should I continue with the patch?

Flags: needinfo?(iireland)

Comment 14

•

4 years ago

Short answer: That code is one place where we should be able to use the new helper.

Long answer: This is a good question. All these branches and labels can be confusing, especially when this code has to be able to handle cond == Equal and cond == NotEqual. (In fact, I managed to confuse myself at least once while writing out this comment.) Let's work through the logic.

The purpose of branchValueIsNurseryCell is to jump to label if:

cond is Equal and the value is in the nursery
cond is NotEqual and the value is not in the nursery.

If we do not jump to label, then we should fall through. That is to say, we should reach the done label.

The first thing we want to do is check whether the value has a type that can be allocated in the nursery. (Note: This is the helper function you are writing.) If the value can't be allocated in the nursery, then we can immediately:

jump to done if cond is Equal
jump to label if cond is NotEqual.

If the value can be allocated in the nursery, then we have to actually check the pointer. That's this code, which you don't have to worry about too much.

So the helper function that we actually want is something more like branchTestNonNurseryType. This helper should take a tag/value/address/baseIndex and a label, and jump to the label if the argument can't be in the nursery. The logic for that looks something like:

  Label canBeInNursery;
  branchTestObject(Equal, ..., canBeInNursery);
  branchTestString(Equal, ..., canBeInNursery);
  branchTestBigInt(NotEqual, ..., label);
  bind(&canBeInNursery);

Then the code in branchValueIsNurseryCell becomes something like:

  Label done;
  branchTestNonNurseryType(address, cond == Assembler::Equal ? &done : label);
 
  loadPtr(ToPayload(address), temp);
  SecondScratchRegisterScope scratch2(*this);
  branchPtrInNurseryChunk(cond, temp, scratch2, label);

  bind(&done);

Does that make sense? (If it doesn't, there's a very real chance that I made a typo/mistake, so don't hesitate to ask if something looks like a blunder on my part.)

As a stretch goal, branchTestNonNurseryType could instead be branchTestNurseryType, with a Condition argument (set to NotEqual in this case). Figuring out the best logic for that version is left as an exercise for the reader. :)

Flags: needinfo?(iireland)

Assignee

Comment 15

•

4 years ago

Can you please explain me in brief how exactly these branchTest functions are working? What is the significance of each parameter and why is the parameters of branchTestBigInt is different from the other two?
Actually, I don't want to simply type the code you gave without understanding it just to submit the patch therefore I'm trying to understand all that is related to this bug.

Flags: needinfo?(iireland)

Comment 16

•

4 years ago

•

Edited

Ignore this. Misclicked.

Status: NEW → RESOLVED

Closed: 4 years ago

Flags: needinfo?(iireland)

Resolution: --- → INVALID

Updated

•

4 years ago

Status: RESOLVED → REOPENED

Resolution: INVALID → ---

Comment 17

•

4 years ago

Sure! These are good questions to have! I'll start from a very high level and work my way down.

SpiderMonkey is a JIT (just-in-time) compiler, which means that it generates machine code at runtime and then executes that code. The macro-assembler is our interface for generating machine instructions. If you dig deep enough, there is code that generates specific instructions for the native architecture (see here, for example), but mostly we try to work at a slightly higher level of abstraction.

branchTestBigInt and friends all follow a similar pattern. They take a Condition, an argument, and a label. They generate code that does the following:

Test the argument. In the case of branchTestBigInt, this means checking whether the argument is a BigInt.
If the result of the comparison matches the Condition, jump to the label.
Otherwise, do nothing and continue executing whatever code is generated next.

So branchTestString(Assembler::Equal, <argument>, label) will jump to label if <argument> is a String, and branchTestString(Assembler::NotEqual, <argument>, label) will jump to label if <argument> is not a String.

(Aside on the syntax for labels: we declare a label when we want to be able to generate an instruction that jumps to it. For example, Label canBeInNursery; in my example above lets us generate the following branchTestObject and branchTestString. To define the actual location of the label, we use bind. We bind canBeInNursery just after the branchTestBigInt, so code that jumps to canBeInNursery will start executing immediately after the code generated by the branchTestBigInt.)

The parameters for branchTestBigInt are different as a slight micro-optimization to save a jump. The more obvious alternative would be something like this:

Label canBeInNursery;
branchTestString(Equal, ..., canBeInNursery)
branchTestObject(Equal, ..., canBeInNursery)
branchTestBigInt(Equal, ..., canBeInNursery)

// If we reach this point, we can't be in the nursery.
jump(label)

bind(&canBeInNursery)

Hopefully that version makes sense? But if you think about it:

if (tag == BigInt) { goto X; } 
goto Y;
X: ...

... does the exact same thing as:

if (tag != BigInt) { goto Y }
X: ...

... except that the latter has one fewer goto.

Does that help?

Assignee

Comment 18

•

4 years ago

Thanks for explaining me all this in detail. I think now I understood what exactly I have to do.
I request you to take a look at the modified version of branchTestNurseryType which I think should work properly and let me know if there is any problem or scope of improvement.

So, in case the condition argument is Equal, then we don't need any new Label and simply jump to label if the value is in nursery (parameters in all 3 functions will be same as I was not able to find any possible optimization in this case).

Then the code in branchTestNurseryType will be:

  Label canBeInNursery;

  branchTestObject(Equal, ..., cond == Assembler::Equal ? label : &canBeInNursery);
  branchTestString(Equal, ..., cond == Assembler::Equal ? label : &canBeInNursery);
  branchTestBigInt(cond, ..., label);

  bind(&canBeInNursery);

And the code in branchValueIsNurseryCell will be almost same as given by you earlier with a small change in line 2:

  Label done;
  branchTestNurseryType(Assembler::NotEqual, address, cond == Assembler::Equal ? &done : label);

  loadPtr(ToPayload(address), temp);
  SecondScratchRegisterScope scratch2(*this);
  branchPtrInNurseryChunk(cond, temp, scratch2, label);

  bind(&done);

Thank you for giving me your precious time :)

Flags: needinfo?(iireland)

Comment 19

•

4 years ago

At first glance, that looks good! The only thing that jumps out to me on first glance is that, now that you have improved the function to work for both Equal and NotEqual, canBeInNursery is no longer a good name for the label.

If you put it up for review on Phabricator, I will take a closer look. (If you have not used Phabricator yet, here are some instructions on getting started.)

Flags: needinfo?(iireland)

Assignee

Comment 20

•

4 years ago

(In reply to Iain Ireland [:iain] from comment #19)

At first glance, that looks good! The only thing that jumps out to me on first glance is that, now that you have improved the function to work for both Equal and NotEqual, canBeInNursery is no longer a good name for the label.

If you put it up for review on Phabricator, I will take a closer look. (If you have not used Phabricator yet, here are some instructions on getting started.)

Should I rename it simply to done or any other better name you suggest?

Flags: needinfo?(iireland)

Comment 21

•

4 years ago

done is good.

Flags: needinfo?(iireland)

Assignee

Comment 22

•

4 years ago

Attached file Bug 1606622 - Add MacroAssembler::branchTestNurseryType. r=sfink,jandem — Details

Phabricator Automation

Updated

•

4 years ago

Assignee: nobody → sanchit.arora.2002

Assignee

Comment 23

•

4 years ago

The patch I submitted 3 days ago still has 'Needs Review' status. Is there something which needs to be done on my part?

Flags: needinfo?(iireland)

Comment 24

•

4 years ago

You don't have to do anything else. Mozilla All-Hands is happening this week in Berlin, so between travel and All-Hands itself, reviews will take longer than normal.

Flags: needinfo?(iireland)

Assignee

Comment 25

•

4 years ago

Sir, sorry for some silly mistakes in the patch but now (as the reviewer said) I have to do something different, that is, check for any-GC-thing now (branchTestGCThing) instead of having a branch for object/string/bigint.
Do you think that I'll be able to do this (I don't want to waste your time explaining each and everything)? If yes, then I'll be very happy to work hard and give time to understand and implement things but will need your guidance a little bit more. If not, can you please give me some bugs which are little bit more challenging than good first bugs but are still for beginners?
Thanks :)

Flags: needinfo?(iireland)