GetIteratorIRGenerator doesn't participate in CacheIR logs

RESOLVED FIXED in Firefox 63

Status

()

defect
P3
normal
RESOLVED FIXED
10 months ago
8 months ago

People

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

Tracking

({good-first-bug})

Trunk
mozilla63
Points:
---

Firefox Tracking Flags

(firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

(Reporter)

Description

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 

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

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!
(Reporter)

Comment 3

10 months ago
mozreview-review
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.
(Reporter)

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.
(Assignee)

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?
(Reporter)

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/jit_test.py --jitflags=all ./dist/bin/js 


https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Build_Documentation
Comment hidden (mozreview-request)
(Assignee)

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:
{
  "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.
(Reporter)

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.
(Reporter)

Comment 10

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

https://reviewboard.mozilla.org/r/253926/#review261362
Attachment #8988719 - Flags: review+
(Reporter)

Updated

10 months ago
Assignee: nobody → petru.gurita1
(Reporter)

Comment 11

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

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
(Reporter)

Comment 14

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

Comment 15

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/aaf899a79269
add 'trackAttached' method to 'GetIteratorIRGenerator' class. r=mgaudet
Keywords: checkin-needed
(Reporter)

Comment 16

10 months ago
Thanks very much for your contribution Petru!

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/aaf899a79269
Status: NEW → RESOLVED
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.