Closed
Bug 1191236
Opened 9 years ago
Closed 9 years ago
Extract methods are unnecessary for operation forwarding on rooting types
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: jonco, Assigned: sfink)
Details
Attachments
(2 files, 1 obsolete file)
36.93 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
Looking at the code I realised that we don't actually need extract() methods for, e.g., getting a Value& from a Rooted<Value>. We already have get() methods on all the relevant types.
Reporter | ||
Comment 1•9 years ago
|
||
Attachment #8643583 -
Flags: review?(terrence)
Comment 2•9 years ago
|
||
Comment on attachment 8643583 [details] [diff] [review] bug1191236-remove-extract-methods Review of attachment 8643583 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Funnily enough, I was thinking exactly the same thing last week, but put off chasing that thread. Thanks for doing the legwork! I'll update the patches in my queue.
Attachment #8643583 -
Flags: review?(terrence) → review+
Comment 4•9 years ago
|
||
sorry had to back this out for hazard failures - > https://treeherder.mozilla.org/logviewer.html#?job_id=12565072&repo=mozilla-inbound https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20150806022659/rootingHazards.txt.gz
Flags: needinfo?(jcoppeard)
Reporter | ||
Comment 5•9 years ago
|
||
So this seems to have made the analysis go crazy and report 8000+ hazards of the form: Function 'blah' has unrooted 'val' of type 'const class JS::Rooted<JS::Value>' live across GC call 'blah' https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20150806022659/rootingHazards.txt.gz I don't /think/ I did anything that changes the behaviour here... Steve could you have a look?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(sphink)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(jcoppeard)
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #5) > So this seems to have made the analysis go crazy and report 8000+ hazards of > the form: > > Function 'blah' has unrooted 'val' of type 'const class > JS::Rooted<JS::Value>' live across GC call 'blah' > > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla- > inbound-linux64-br-haz/20150806022659/rootingHazards.txt.gz > > I don't /think/ I did anything that changes the behaviour here... Steve > could you have a look? Finally got around to looking at this. Looks dumb, dumb, dumb. I believe it is complaining about the type 'const class JS::Rooted<JS::Value>' not being a rooted type, because the canonicalization code (stripUCSAndNamespace) expects 'class' to come before 'const'. :-( I don't know that order would change with this patch. Or perhaps it didn't, but now the 'const' is coming through where before it wasn't?
Flags: needinfo?(sphink)
Assignee | ||
Comment 8•9 years ago
|
||
Oh boy, let's see if this uploads the right patch. Anyway, pushing to try with a fix for this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e13235add54
Assignee | ||
Updated•9 years ago
|
Assignee: jcoppeard → sphink
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•9 years ago
|
||
This fixed the problem in my local run.
Attachment #8650697 -
Flags: review?(jcoppeard)
Reporter | ||
Comment 10•9 years ago
|
||
Comment on attachment 8650697 [details] [diff] [review] Fix UCS canonicalization Review of attachment 8650697 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!
Attachment #8650697 -
Flags: review?(jcoppeard) → review+
Reporter | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a66adecf152
Assignee | ||
Comment 12•9 years ago
|
||
Sorry, I should have pushed all of that in the first place. I just checked some of the hazard builds, and they're good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f494e6bcb413
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8649023 [details] [diff] [review] Fix UCS canonicalization I had a bug in my new mq-less bzexport attachment obsoleting code. Now fixed.
Attachment #8649023 -
Attachment is obsolete: true
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/da51ae717dcb https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c625817205
Comment 15•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/da51ae717dcb https://hg.mozilla.org/mozilla-central/rev/a7c625817205
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•