Implement CodeGenerator::DataPtr

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla26
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 796322 [details] [diff] [review]
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.
Attachment #796322 - Flags: review+
(Assignee)

Comment 1

5 years ago
Created attachment 796419 [details] [diff] [review]
Part 0: Use runtimeData_ offsets instead of cacheList_ offsets to refer to caches other than during GC
Assignee: general → efaustbmo
Status: NEW → ASSIGNED
Attachment #796419 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

5 years ago
Created attachment 796420 [details] [diff] [review]
Part 2: Move visit*IC to taking DataPtr<IC> instead of IC *
Attachment #796420 - Flags: review?(nicolas.b.pierron)
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+.
Attachment #796419 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

5 years ago
Created attachment 796823 [details] [diff] [review]
Part 1: Implement DataPtr, and use it in addCache()
Attachment #796322 - Attachment is obsolete: true
Attachment #796823 - Flags: review+
(Assignee)

Comment 5

5 years ago
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.
Attachment #796419 - Flags: review?(nicolas.b.pierron)
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.
(Assignee)

Comment 7

5 years ago
(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.
Attachment #796419 - Flags: review?(nicolas.b.pierron) → review+
Attachment #796420 - Flags: review?(nicolas.b.pierron) → review+
https://hg.mozilla.org/mozilla-central/rev/8328d4b74a2e
https://hg.mozilla.org/mozilla-central/rev/674a3e4f0959
https://hg.mozilla.org/mozilla-central/rev/125ce2b2c1a8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.