Closed Bug 994018 Opened 6 years ago Closed 5 years ago

TO: TypedObject assignment should not use Memcpy


(Core :: JavaScript Engine, defect)

Not set





(Reporter: lth, Assigned: jschulte)




(1 file, 3 obsolete files)

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.
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).
As proposed in , this should disable the memcpy-optimization for opaque Typed Objects.
Attachment #8454922 - Flags: review?(nmatsakis)
Attachment #8454922 - Flags: approval-mozilla-esr24?
Attachment #8454922 - Flags: approval-mozilla-esr24?
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 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 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)
Attached patch v3.patch (obsolete) — Splinter Review
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 on attachment 8473732 [details] [diff] [review]

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+
Attached patch v4.patchSplinter Review
Green on try:
Assignee: nobody → j_schulte
Attachment #8473732 - Attachment is obsolete: true
Attachment #8491725 - Flags: review+
Keywords: checkin-needed
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.