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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jonco, Assigned: sfink)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attachment #8643583 - Flags: review?(terrence)
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+
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?
Flags: needinfo?(sphink)
Flags: needinfo?(jcoppeard)
(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)
Attached patch Fix UCS canonicalization (obsolete) — Splinter Review
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: jcoppeard → sphink
Status: NEW → ASSIGNED
This fixed the problem in my local run.
Attachment #8650697 - Flags: review?(jcoppeard)
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+
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
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
https://hg.mozilla.org/mozilla-central/rev/da51ae717dcb
https://hg.mozilla.org/mozilla-central/rev/a7c625817205
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.