GenerationalGC: Hashtable implementation puts heap-only data on the stack

RESOLVED DUPLICATE of bug 989013

Status

()

Core
JavaScript Engine
RESOLVED DUPLICATE of bug 989013
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The GGC store buffer records the address of heap locations containing pointers into the nursery.  We do this automatically when a RelocatablePtr<T> is written to, using operator overloading.

Since hashtable values are stored on the heap, if GC thing pointers are stored in hash tables they must be wrapped in a RelocatablePtr<T>.  Such values can't be used on the stack because writing to them will store a pointer to their location in the store buffer, and for stack locations this will be invalid by the time the store buffer is traced.

Unfortunately, the hashtable implementation does create value objects on the stack in a couple of places - while adding to the hashtable, and while rekeying. This is one of the reasons behind bug 913224.
I believe this particular usage to be safe: ~RelocatablePtr<T> "removes" the stack location from the store buffer so as long as we cannot GC while the RelocatablePtr<T> is on the stack, the next MinorGC should not be able to observe a stack location in the buffer.
(Assignee)

Comment 2

4 years ago
I linked the wrong bug - this is showing up in a GC triggered from the operation callback in bug 913261.
Blocks: 913261
No longer blocks: 913224
(Assignee)

Comment 3

4 years ago
Actually bug 913261 is not caused by this either.  But I still think it's nasty to put stack locations in the storebuffer, even if it's only temporarily.  Apart from anything else it's a waste of space.  Anyway, maybe this is something to look at when we come to optimizing GGC.
No longer blocks: 913261
Yeah, I totally agree, if we can do it cleanly. It is the way it is because it trades a bit of runtime complexity for significantly simpler implementation. Other options we considered are:

1) Have a custom BufferableRefs for all hash tables with associated gunk at every put.
2) Split the interface and heap types in HashTable and Vector.
3) Teach HashTable and Vector about the store buffer, presumably with a template argument.

The first is easy to implement but impossible to maintain. The second would require an unacceptably large amount of extra type complexity in HashTable and Vector. The third was shot down by Luke before it got off the ground because of the extra template argument.

Luckily, the relocatable buffers do not appear to be heavily used on any of the benchmarks I have looked at yet. Or alternatively, we used BufferableRefs instead of RelocatablePtr in all of the tables we know are hot.
(Assignee)

Comment 5

4 years ago
Created attachment 814913 [details] [diff] [review]
watchpoint-fix

Here's a patch to fix the places in jswatchpoint.cpp where we explicitly create these one the stack.
Attachment #814913 - Flags: review?(terrence)
(Assignee)

Updated

4 years ago
Whiteboard: [leave open]
Comment on attachment 814913 [details] [diff] [review]
watchpoint-fix

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

Reasonable. r=me

::: js/src/jswatchpoint.cpp
@@ +51,5 @@
>  } /* anonymous namespace */
>  
> +/*
> + * Watchpoint contains a RelocatablePtrObject member, which is conceptually a
> + * heap-only class.  It's preferable not to allocate these on the stack as they

One space between sentences.
Attachment #814913 - Flags: review?(terrence) → review+
(Assignee)

Updated

4 years ago
Blocks: 885550
I think bug 989013 is probably the best way to fix this in the long term.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 989013
You need to log in before you can comment on or make changes to this bug.