1195568 - Mark Heap<T> as MOZ_HEAP_CLASS

Status: RESOLVED WONTFIX

(Core :: JavaScript: GC, defect)

Description

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

Now that we have MOZ_HEAP_CLASS, we ought to use it!

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Mark Heap<T> as MOZ_HEAP_CLASS

I figured I'd write the 1-line patch ^.^

There are, as you'd expected, a bunch of failures. I'll open bugs as I come across them which block this one.

Comment 2

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

Awesome, thanks!

Terrence, what are all the types where we'd like to have this? HeapPtr<T> but not RelocatablePtr<T>?

Comment 3

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

Comment on attachment 8649426 [details] [diff] [review]
Mark Heap<T> as MOZ_HEAP_CLASS

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

This should be at least extended to TenuredHeap<T>. If we can clear out the dependencies, it would be good to land this before extending it to other types.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to Steve Fink [:sfink, :s:] from comment #3)
> Comment on attachment 8649426 [details] [diff] [review]
> Mark Heap<T> as MOZ_HEAP_CLASS
> 
> Review of attachment 8649426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This should be at least extended to TenuredHeap<T>. If we can clear out the
> dependencies, it would be good to land this before extending it to other
> types.

Yeah I think that it makes sense to do this one type at a time. We should clear out these dependents for Heap<T>, and then I can run the build again and see if there's anything left.

Comment 5

[Terrence Cole [:terrence]]

(In reply to Steve Fink [:sfink, :s:] from comment #2)
> Awesome, thanks!
> 
> Terrence, what are all the types where we'd like to have this? HeapPtr<T>
> but not RelocatablePtr<T>?

Well, RelocatablePtr has exactly the same semantics as Heap<T>, so I'd think we'd want it to have a heap annotation as well. I think it would be nice to have it on all 4 pointer wrappers in js/src/gc/Barrier.h. This may be a bit difficult because of HashTable: we pass things to the hashtable as parameters and some of those are inevitably going to be heap types. My work in bug 1194544 should make this all substantially simpler as well be using exclusively RelocatablePtr with HashTable then.

Comment 6

[Jon Coppeard (:jonco)]

Following discussion with terrence and sfink yesterday, it sounds like it's more useful to allow Heap<T> to be allocated on the stack.  It is safe, the performance penalty is very small, and it makes it possible to write clearer code.