1191236 - Extract methods are unnecessary for operation forwarding on rooting types

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco) (PTO until 2nd March)]

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.

Comment 1

[Jon Coppeard (:jonco) (PTO until 2nd March)]

Attachment: bug1191236-remove-extract-methods

Comment 2

[Terrence Cole [: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.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f861148c83c

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[Jon Coppeard (:jonco) (PTO until 2nd March)]

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?

Comment 6

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71263add546e

Comment 7

[Steve Fink [:sfink] [:s:]]

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

Comment 8

[Steve Fink [:sfink] [:s:]]

Attachment: Fix UCS canonicalization

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

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Fix UCS canonicalization

This fixed the problem in my local run.

Comment 10

[Jon Coppeard (:jonco) (PTO until 2nd March)]

Comment on attachment 8650697 [details] [diff] [review]
Fix UCS canonicalization

Review of attachment 8650697 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 11

[Jon Coppeard (:jonco) (PTO until 2nd March)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a66adecf152

Comment 12

[Steve Fink [:sfink] [:s:]]

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 13

[Steve Fink [:sfink] [:s:]]

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.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da51ae717dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7c625817205

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da51ae717dcb
https://hg.mozilla.org/mozilla-central/rev/a7c625817205