Mark Heap<T> as MOZ_HEAP_CLASS

RESOLVED WONTFIX

Status

()

Core
JavaScript: GC
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: sfink, Assigned: mystor)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
Now that we have MOZ_HEAP_CLASS, we ought to use it!
(Assignee)

Comment 1

2 years ago
Created attachment 8649426 [details] [diff] [review]
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.
Attachment #8649426 - Flags: review?(sphink)
(Assignee)

Updated

2 years ago
Depends on: 1195951
(Assignee)

Updated

2 years ago
Depends on: 1195957
(Reporter)

Comment 2

2 years ago
Awesome, thanks!

Terrence, what are all the types where we'd like to have this? HeapPtr<T> but not RelocatablePtr<T>?
Flags: needinfo?(terrence)
(Assignee)

Updated

2 years ago
Depends on: 1195972
(Reporter)

Comment 3

2 years ago
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.
Attachment #8649426 - Flags: review?(sphink) → review+
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Updated

2 years ago
Assignee: nobody → michael
(Reporter)

Updated

2 years ago
Blocks: 1008341
(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.
Flags: needinfo?(terrence)
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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.