Closed Bug 995442 Opened 6 years ago Closed 6 years ago

Don't fire barriers in the browser for non-object Heap<T>

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
flash10

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(1 file)

C++ should choose the most specific template. If it doesn't, the try run below should break thoroughly.

https://tbpl.mozilla.org/?tree=Try&rev=27e7ffff1472
Attachment #8405613 - Flags: review?(sphink)
Comment on attachment 8405613 [details] [diff] [review]
dont_barrier_non_objects-v0.diff

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

Seems fine to me, but I'd sort of like it to be in the other order -- generic template <typename T> first, followed by template <JSObject *>. It shouldn't make a difference to the template resolution order, and I think it's more customary to define the general case and then list out the exceptions. (Even if, semantically, the barrier code for JSObject* is actually more general in this case.)
Attachment #8405613 - Flags: review?(sphink) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba77af4dbd10

Failures here appear to be the crazyness yesterday and not symptomatic of missing barriers:
https://tbpl.mozilla.org/?tree=Try&rev=27e7ffff1472

(In reply to Steve Fink [:sfink] from comment #1)
> Comment on attachment 8405613 [details] [diff] [review]
> dont_barrier_non_objects-v0.diff
> 
> Review of attachment 8405613 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems fine to me, but I'd sort of like it to be in the other order --
> generic template <typename T> first, followed by template <JSObject *>. It
> shouldn't make a difference to the template resolution order, and I think
> it's more customary to define the general case and then list out the
> exceptions. (Even if, semantically, the barrier code for JSObject* is
> actually more general in this case.)

You are quite right! My thinking writing the patch was: narrow the original and add a new, non-functional generic. It's interesting to see how that approach led me to implement something which is so obviously wrong, stylistically. Nice also to see how the review process sometimes even works -- ignoring how I missed the bug in the original Heap<T> patch.
https://hg.mozilla.org/mozilla-central/rev/ba77af4dbd10
Assignee: nobody → terrence
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 989414
You need to log in before you can comment on or make changes to this bug.