Make Bindings use the normal Rooted infrastructure

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
It's currently using the wonky InternalHandle thing because we wanted to avoid a bunch of extraneous stack copies. What I didn't realize at the time is that the actual Binding* is external, so the struct itself is a svelt 40 bytes. Copying these to and from the stack is not ruinous at all, allows us to use normal Rooted techniques, and will allow us to remove InternalHandle.
(Assignee)

Comment 1

3 years ago
Created attachment 8631635 [details] [diff] [review]
cleanup_bindings_rooting-v0.diff
Attachment #8631635 - Flags: review?(shu)

Comment 2

3 years ago
Comment on attachment 8631635 [details] [diff] [review]
cleanup_bindings_rooting-v0.diff

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

This is a nice cleanup.

My only dislike is that almost always we pull the Bindings out only to copy it back into JSScript, largely due to this temporary vs script storage nonsense. It'd be nice to clean that up so we have some stack-allocated Bindings that get copied to script storage wholesale instead of having its "storage switched".

If we don't switch that soon, it would be nice to have some RAII thing that combines the rooting + copying back into JSScript.
Attachment #8631635 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/d588ff0a68d9
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox42: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Status: RESOLVED → REOPENED
Flags: needinfo?(terrence)
Resolution: FIXED → ---
(Assignee)

Comment 6

3 years ago
No change after backout. The regression appears to be noise.
Flags: needinfo?(terrence)
https://hg.mozilla.org/mozilla-central/rev/de6a79687e70
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.