Use virtual dispatch to simplify PersistentRooted

RESOLVED INVALID

Status

()

Core
JavaScript: GC
RESOLVED INVALID
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox41 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8621324 [details] [diff] [review]
2_make_persistentrooted_generic-v0.diff

We currently have a ton of copy-paste for each type that can use PersistentRooted to store the list heads and call the relevant trace function. Rather than having umpteen different lists, I think it would be better to just eat the extra cost from making it virtual in favor of a simpler implementation. The cost, specifically, should be one extra word in PersistentRooted 3->4 and virtual dispatch when tracing. Given that these are roots, they should not, by definition, be a significant fraction of our heap or tracing overhead, so I don't think this will be problematic. 

As to mechanism: this patch adds a TraceableListElement with virtual trace and finish methods. The JSRuntime then contains a list of TraceableListElements. PersistentRooted then becomes an implementation of TraceableListElement that calls TraceRoot on its pointer.

Aside from saving ~70 lines of copy-paste, this also sets us up with an extremely generic mechanism for future rooting. Right now what one does to add roots to the system involves an unspeakably ridiculous amount of boilerplate and pain. Enough that I think this mechanism will eventually pay for itself in binary size and icache savings, despite the virtual trace calls.
Attachment #8621324 - Flags: review?(jimb)
(Assignee)

Comment 1

3 years ago
Comment on attachment 8621324 [details] [diff] [review]
2_make_persistentrooted_generic-v0.diff

I had a better idea for this on the bus ride from Whistler to Vancouver.
Attachment #8621324 - Flags: review?(jimb)
(Assignee)

Comment 2

3 years ago
Okay, StaticTraceable and DynamicTraceable are a thing now and work well with Rooted. I have a series of patches coming up that adds these capabilities to PersistentRooted, so it looks like this is totally superseded.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.