909989 - Implement CodeGenerator::DataPtr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Attachment: DataPtr.patch

Currently, putting things in CodeGenerator::runtimeData_ is a huge footgun. Every time you append to the vector, it might realloc, causing any pointer to an object stored in the same vector to immediately become stale.

I propose a templated wrapper, DataPtr, which wraps the offset into the vector, and actively does the vector calculations at every access.

Attached in a first draft, previously reviewed.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: Use runtimeData_ offsets instead of cacheList_ offsets to refer to caches other than during GC

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 2: Move visit*IC to taking DataPtr<IC> instead of IC *

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 796419 [details] [diff] [review]
Part 0: Use runtimeData_ offsets instead of cacheList_ offsets to refer to caches other than during GC

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

I am not sure to understand where this patch should be applied, update the patches which are attached to this bug, either this one, or the one which carry the r+.

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 1: Implement DataPtr, and use it in addCache()

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 796419 [details] [diff] [review]
Part 0: Use runtimeData_ offsets instead of cacheList_ offsets to refer to caches other than during GC

This should apply to tip. It has no other dependencies.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 796823 [details] [diff] [review]
Part 1: Implement DataPtr, and use it in addCache()

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

::: js/src/jit/CodeGenerator.cpp
@@ +118,5 @@
>  
>  bool
>  CodeGenerator::visitOutOfLineCache(OutOfLineUpdateCache *ool)
>  {
> +    DataPtr<IonCache> cache(this, ool->getCacheIndex());

Ok, now that I have seen this change, I can accept the part 0.

Comment 7

[Eric Faust [:efaust]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Comment on attachment 796823 [details] [diff] [review]
> Part 1: Implement DataPtr, and use it in addCache()
> 
> Review of attachment 796823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CodeGenerator.cpp
> @@ +118,5 @@
> >  
> >  bool
> >  CodeGenerator::visitOutOfLineCache(OutOfLineUpdateCache *ool)
> >  {
> > +    DataPtr<IonCache> cache(this, ool->getCacheIndex());
> 
> Ok, now that I have seen this change, I can accept the part 0.

Yeah, sorry. This was not generally well done on my part. I had this mental model of "oh, yeah, and then I'll do the trivial rebase, and it'll be fine. He'll 'just know'", and of course that makes no sense.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc30b7c2277
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1842e91034d
https://hg.mozilla.org/integration/mozilla-inbound/rev/91b14e27101a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8328d4b74a2e
https://hg.mozilla.org/mozilla-central/rev/674a3e4f0959
https://hg.mozilla.org/mozilla-central/rev/125ce2b2c1a8