1468750 - GetIteratorIRGenerator doesn't participate in CacheIR logs

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #1468749 +++

Unlike most of the IR Generators in js/src/jit/CacheIR.cpp, GetIteratorIRGenerator appears not to participate in the logging provided by running with a debug build and CACHEIR_LOGS=1 

Tasks: 

* Add a trackAttached method to GetIteratorIRGenerator that reports its const char* argument to the cacheIR spewer, along with the value of val_, which is what the IR generator is using to make its attachment decisions. This can be modeled on other CacheIR trackAttached methods. 
* Add calls to the newly added trackAttached to name the caches that successfully attach. 
* Verify that the logging works by doing a debug build of spidermonkey, and running a test case with the environment variable CACHEIR_LOGS=1, and checking /tmp/cacheir_logs.* for output from GetIteratorIRGenerator.

Comment 1

[Petru Gurita]

Attachment: Bug 1468750 - add 'trackAttached' method to 'GetIteratorIRGenerator' class.

Added method 'trackAttached' to the class 'GetIteratorIRGenerator' and implemented it accordingly.

Review commit: https://reviewboard.mozilla.org/r/253926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253926/

Comment 2

[Petru Gurita]

Hello, I'll give it a shot. I tried to complete the first two tasks. I used the other classes from IRGenerator as models. I am waiting for feedback. Thanks!

Comment 3

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

Comment on attachment 8988719 [details]
Bug 1468750 - add 'trackAttached' method to 'GetIteratorIRGenerator' class.

https://reviewboard.mozilla.org/r/253926/#review260666

::: js/src/jit/CacheIR.cpp:4403
(Diff revision 1)
> +GetIteratorIRGenerator::trackAttached(const char* name)
> +{
> +#ifdef JS_CACHEIR_SPEW
> +    if (const CacheIRSpewer::Guard& sp = CacheIRSpewer::Guard(*this, name)) {
> +        sp.valueProperty("val", val_);
> +        sp.valueProperty("property", StringValue(name_));

I don't think we want this name, as I'm not sure it's the name you think. 

I think this line can be dropped.

Comment 4

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

When you get back to this, it would be nice to have one of the new emitted log entries posted here. 

The best way I can think of to find a test case is to go to where you are calling trackAttached, and append a MOZ_CRASH("hit"); this will crash the program when a test case hits your code. You can then remove the MOZ_CRASH, and run the same test case in debug with CACHEIR_LOGS=1.

Comment 5

[Petru Gurita]

Ok then. I'll remove the last line of code, the one that refers to `name_` . Regarding testing, I'm not sure how the program is called it in order to make it crash in the first place. Should I check SpiderMonkey?

Comment 6

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

You can build SpiderMonkey alone following the instructions here [1], and run the jit test suite. 

I would do 

    mkdir js/src/build_OPT.OBJ
    cd js/src/build_OPT.OBJ
    ../configure --enable-warnings-as-errors --disable-tests --enable-optimize --disable-debug  --enable-jitspew
    ../jit-test/jit_test.py --jitflags=all ./dist/bin/js 


https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation

Comment 7

[Petru Gurita]

Comment on attachment 8988719 [details]
Bug 1468750 - add 'trackAttached' method to 'GetIteratorIRGenerator' class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/253926/diff/1-2/

Comment 8

[Petru Gurita]

Hello, thanks for the detailed explanation! I did the debugging session and everything worked just as expected. Here is one example of a JSON entry that contains a `GetIteratorIRGenerator` log:
{
  "name":"GetIterator",
  "file":"/home/petru/firefox/mozilla-central/js/src/jit-test/tests/auto-regress/bug499169.js",
  "mode":0,
  "line":91,
  "column":0,
  "pc":"7f99c786a626",
  "val":{
    "type":"Object",
    "value":"7f99ca400f40 (shape: 7f99c77acc18)"
  },
  "attached":"GetIterator"
}

I also updated my patch.

Comment 9

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

Looks good to me :) 

I have triggered a try build (my first time via mozreview, so we'll see how that goes). Assuming it's green, I'll circle back and r+ the patch.

Comment 10

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

Comment on attachment 8988719 [details]
Bug 1468750 - add 'trackAttached' method to 'GetIteratorIRGenerator' class.

https://reviewboard.mozilla.org/r/253926/#review261362

Comment 11

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

So, at this point you should be able to set the checkin-needed flag on the bug.

Comment 12

[Petru Gurita]

Sorry, I'm not sure how to set a checkin-needed flag on the bug. I can't find it in the Bug Flags menu.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

It's a bug keyword :)

Comment 14

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

:D Thanks Ryan! Sniped my in-flight comment.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aaf899a79269
add 'trackAttached' method to 'GetIteratorIRGenerator' class. r=mgaudet

Comment 16

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

Thanks very much for your contribution Petru!

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/aaf899a79269