Closed
Bug 994018
Opened 11 years ago
Closed 10 years ago
TO: TypedObject assignment should not use Memcpy
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: lth, Assigned: jschulte)
References
Details
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
jschulte
:
review+
|
Details | Diff | Splinter Review |
Currently the TO system uses Memcpy to copy the bits of one TypedObject to another when the descriptors of the two objects match; it is the fast, common path. That optimization runs afoul generational GC if the type contains a reference, the destination object D is tenured, and the source object S contains a pointer to another object X that is not tenured, since in that case the D -> X edge would have to be added to the remembered set. So far as I can tell it is not.
The consequence of the bug is that X may not be promoted and the D -> X pointer may become wild / dangling.
The thing that might save us from a crash is if the object D is in the remembered set in its entirety, which could happen if it's subject to other assignments.
It'd be fairly involved to demonstrate this with a test case and I don't have such a test case.
Comment 1•11 years ago
|
||
Agreed there is a problem. The easiest way to fix is just limiting memcpy calls to types which do not contain object references (transparent types).
Assignee | ||
Comment 2•10 years ago
|
||
As proposed in https://bugzilla.mozilla.org/show_bug.cgi?id=994018#c1 , this should disable the memcpy-optimization for opaque Typed Objects.
Attachment #8454922 -
Flags: review?(nmatsakis)
Attachment #8454922 -
Flags: approval-mozilla-esr24?
Assignee | ||
Updated•10 years ago
|
Attachment #8454922 -
Flags: approval-mozilla-esr24?
Assignee | ||
Comment 3•10 years ago
|
||
Could this also be an approach to the problem? This way we could keep the optimization.
P.S. Sorry for the esr request, hit the wrong button.
Attachment #8454923 -
Flags: feedback?(nmatsakis)
Comment 4•10 years ago
|
||
Comment on attachment 8454922 [details] [diff] [review]
Disable memcpy-optimization for opaque TO
Review of attachment 8454922 [details] [diff] [review]:
-----------------------------------------------------------------
I'm sorry for taking so long on this. This looks fine, though I think it might make sense to just remove the memcopy "optimization" altogether and add it back later if it seems important.
Attachment #8454922 -
Flags: review?(nmatsakis) → review+
Comment 5•10 years ago
|
||
Comment on attachment 8454923 [details] [diff] [review]
Trigger postBarriers for References, when memcpy'ing Typed Objects
Review of attachment 8454923 [details] [diff] [review]:
-----------------------------------------------------------------
This patch looks questionable. For one thing, I would expect to use the type of the target (though it probably doesn't matter, as the two are the same). But more importantly, the type may be an array of structs or something like that. There is a function somewhere that is used to visit all references (it's used when tracing), we could potentially employ that here.
Attachment #8454923 -
Flags: feedback?(nmatsakis)
Assignee | ||
Comment 6•10 years ago
|
||
As discussed on IRC.
In case, this is r+, I would appreciate, if you could also try-run this for me, as I don't have the necessary commit level.
Attachment #8454922 -
Attachment is obsolete: true
Attachment #8454923 -
Attachment is obsolete: true
Attachment #8473732 -
Flags: review?(nmatsakis)
Comment 7•10 years ago
|
||
Comment on attachment 8473732 [details] [diff] [review]
v3.patch
Review of attachment 8473732 [details] [diff] [review]:
-----------------------------------------------------------------
r+ except for the missing break statements.
::: js/src/builtin/TypedObject.js
@@ +344,5 @@
> + case JS_X4TYPEREPR_FLOAT32:
> + Store_float32(typedObj, offset + 0, Load_float32(fromValue, 0));
> + Store_float32(typedObj, offset + 4, Load_float32(fromValue, 4));
> + Store_float32(typedObj, offset + 8, Load_float32(fromValue, 8));
> + Store_float32(typedObj, offset + 12, Load_float32(fromValue, 12));
missing a break here?
@@ +350,5 @@
> + case JS_X4TYPEREPR_INT32:
> + Store_int32(typedObj, offset + 0, Load_int32(fromValue, 0));
> + Store_int32(typedObj, offset + 4, Load_int32(fromValue, 4));
> + Store_int32(typedObj, offset + 8, Load_int32(fromValue, 8));
> + Store_int32(typedObj, offset + 12, Load_int32(fromValue, 12));
and here?
Attachment #8473732 -
Flags: review?(nmatsakis) → review+
Assignee | ||
Comment 8•10 years ago
|
||
Green on try:
https://tbpl.mozilla.org/?tree=Try&rev=9c4886be3fcf
Assignee: nobody → j_schulte
Attachment #8473732 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8491725 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•