GetIteratorIRGenerator doesn't participate in CacheIR logs

RESOLVED FIXED in Firefox 63



10 months ago
8 months ago


(Reporter: mgaudet, Assigned: petru.gurita1, Mentored)




Firefox Tracking Flags

(firefox62 wontfix, firefox63 fixed)


(Whiteboard: [lang=c++])


(1 attachment)



10 months ago
+++ 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 


* 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.
Priority: -- → P3
Whiteboard: [lang=c++]
Comment hidden (mozreview-request)

Comment 2

10 months ago
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

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

::: js/src/jit/CacheIR.cpp:4403
(Diff revision 1)
> +GetIteratorIRGenerator::trackAttached(const char* name)
> +{
> +    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

10 months ago
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

10 months ago
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

10 months ago
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/ --jitflags=all ./dist/bin/js
Comment hidden (mozreview-request)

Comment 8

10 months ago
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:
    "value":"7f99ca400f40 (shape: 7f99c77acc18)"

I also updated my patch.

Comment 9

10 months ago
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

10 months ago
Comment on attachment 8988719 [details]
Bug 1468750 - add 'trackAttached' method to 'GetIteratorIRGenerator' class.
Attachment #8988719 - Flags: review+


10 months ago
Assignee: nobody → petru.gurita1

Comment 11

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

Comment 12

10 months ago
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.
It's a bug keyword :)
Keywords: checkin-needed

Comment 14

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

Comment 15

10 months ago
Pushed by
add 'trackAttached' method to 'GetIteratorIRGenerator' class. r=mgaudet
Keywords: checkin-needed

Comment 16

10 months ago
Thanks very much for your contribution Petru!

Comment 17

10 months ago
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.