1493189 - Collapse ICCacheIR_UpdatedStub into ICUpdatedStub

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

It appears that the CacheIR version of the stub has now completely subsumed the base class, and to the distinction no longer makes sense. 

We can collapse the two, simplifying the code.

As well, when the collapse happens we have an opportunity to potentially pack fields and save memory

Comment 1

[Adam Holm]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #0)
> It appears that the CacheIR version of the stub has now completely subsumed
> the base class, and to the distinction no longer makes sense. 
> 
> We can collapse the two, simplifying the code.
> 
> As well, when the collapse happens we have an opportunity to potentially
> pack fields and save memory

How difficult of a fix would you say this is? I'd like to work on this bug.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Should be relatively easy: Should just be a matter of smooshing (scientific term) the two classes together, and renaming all the uses of the old class. 

You'd be combining this [1] with this [2]. I'd use the derived class name everywhere. 

When you do the smoosh, you'll want to arrange the fields in order of size to improve the changes they get packed: Order them from largest to smallest. 

[1]: https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/src/jit/BaselineIC.h#920
[2]: https://searchfox.org/mozilla-central/rev/f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/src/jit/BaselineIC.h#853

Comment 3

[Adam Holm]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #2)
> 
> When you do the smoosh, you'll want to arrange the fields in order of size
> to improve the changes they get packed: Order them from largest to smallest. 

Do you mean size in terms of size of the variable type, e.g. |sizeof(GCPtrObjectGroup)|?

> [1]:
> https://searchfox.org/mozilla-central/rev/
> f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/src/jit/BaselineIC.h#920
> [2]:
> https://searchfox.org/mozilla-central/rev/
> f2028b4c38bff2a50ed6aa1763f6dc5ee62b0cc4/js/src/jit/BaselineIC.h#853

Should I be doing the work on this specific revision? Or doing it on the current tip of mozilla-central okay?

Comment 4

[Mayank Bansal]

:mgaudet is off work for some time. ni other JS folks.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Adam Holm from comment #3)
> Do you mean size in terms of size of the variable type, e.g.
> |sizeof(GCPtrObjectGroup)|?

Yep.

> Should I be doing the work on this specific revision? Or doing it on the
> current tip of mozilla-central okay?

mozilla-central tip would be best.

Comment 6

[Adam Holm]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Adam Holm from comment #3)
> > Do you mean size in terms of size of the variable type, e.g.
> > |sizeof(GCPtrObjectGroup)|?
> 
> Yep.
> 
> > Should I be doing the work on this specific revision? Or doing it on the
> > current tip of mozilla-central okay?
> 
> mozilla-central tip would be best.

Thanks!

Comment 7

[Adam Holm]

Attachment: bug1493189.patch

Here is what I have so far. I have this code commented out https://searchfox.org/mozilla-central/source/js/src/jit/BaselineIC.h#609-617, the patch doesn't work otherwise, but I'm working on getting it to work without that code commented out.

I'll also attach the diff'ed output of jit-test/jit_test.py build_DBG.OBJ/dist/bin/js run with and without these fixes, and the diff'ed output of tests/jstests.py build_DBG.OBJ/dist/bin/js run with and without these fixes.

Any feedback is appreciated, thanks!

Comment 8

[Adam Holm]

Attachment: diff'ed output from running `jit-test/jit_test.py build_DBG.OBJ/dist/bin/js` with and without fixes

Comment 9

[Adam Holm]

Attachment: diff'ed output from running `tests/jstests.py build_DBG.OBJ/dist/bin/js` with and without fixes

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Adam Holm from comment #8)

Created attachment 9037807 [details]
diff'ed output from running jit-test/jit_test.py build_DBG.OBJ/dist/bin/js with and without fixes

Why do you get so many test failures even without the patch?

All tests should pass without the patch.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 9037806 [details] [diff] [review]
bug1493189.patch

Review of attachment 9037806 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this.

The patch makes sense, but it would be better and simpler to fold ICUpdatedStub into ICCacheIR_Updated instead of the other way around. That way you don't have to change the stub kind etc.

Comment 12

[Adam Holm]

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

(In reply to Adam Holm from comment #8)
Why do you get so many test failures even without the patch?

All tests should pass without the patch.

This is a very good point, haha. Thanks for pointing this out!

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

Thanks for working on this.

The patch makes sense, but it would be better and simpler to fold
ICUpdatedStub into ICCacheIR_Updated instead of the other way around. That
way you don't have to change the stub kind etc.

I'll implement this after I address the above test issue. Thank you for the suggestion and the great feedback!

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Adam Holm from comment #12)

I'll implement this after I address the above test issue. Thank you for the suggestion and the great feedback!

Great! Let me know if you need help with the failing tests. It's curious they fail on your machine.

Comment 14

[Adam Holm]

Attachment: Bug 1493189 - Collapse ICCacheIR_UpdatedStub into ICUpdatedStub. r=jandem

Collapsed ICUpdatedStup into ICCacheIR_Updated.

Comment 15

[Adam Holm]

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

(In reply to Adam Holm from comment #12)

I'll implement this after I address the above test issue. Thank you for the suggestion and the great feedback!

Great! Let me know if you need help with the failing tests. It's curious they fail on your machine.

I've submitted my patch implementing the folding you suggested to Phabricator (let me know if I did anything incorrectly in that process). I ran the jstests.py and jit_test.py tests with the build_DBG.OBJ/dist/bin/js again and didn't run into the same errors, so I'm not sure what caused them. There were a handful of timeouts, but when I ran those test files individually, they passed. I also ran the benchmarks described here: https://wiki.mozilla.org/JavaScript:New_to_SpiderMonkey#Benchmark_your_changes, and got good results on my machine.

Comment 16

[Matthew Gaudet (he/him) [:mgaudet]]

(Seems a patch went arwy; see phabricator comments!)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9de2b24ac949
Collapse ICCacheIR_UpdatedStub into ICUpdatedStub. r=mgaudet

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/9de2b24ac949