Closed Bug 898362 Opened 11 years ago Closed 11 years ago

[binary data] self-host where possible

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(1 file, 3 obsolete files)

I've given up on my original dream of implementing ALL the binary data semantics using self-hosting, but it should still be possible to self-host most or all of the various utility functions. This will require at least a new memcpy intrinsic that allows us to move data around, and possibly a few others. Self-hosting will of course make execution more efficient (no C++ to JS barrier, etc) but also permits those functions to be used in parallel execution, presuming we implement the intrinsics properly.
Blocks: 898342
Convert from enums to #defines. Also convert from JSObject* and HandleObject to JSObject& in many cases. These two changes are both rote and somewhat entangled with respect to the lines of code they touch.
Assignee: general → nmatsakis
Attached patch Bug898362-Part2-SelfHost.diff (obsolete) — Splinter Review
Move ConvertAndCopyTo and binary fill into self-hosted code. More to come.
Attached patch Bug898362-Part3-AdjustTests.diff (obsolete) — Splinter Review
Adjust tests. There is a discrepancy between our current typed arrays, which convert `[]` to `NaN`, and the spec function `ToNumber()`, which seems to convert `[]` to `0` (or at least `+[]` is `0`). I have to consult with dherman, but I believe our typed arrays are not obeying the spec in this regard and should (eventually) be adjustd.
Attachment #802361 - Flags: review?(till)
Adding dependency on bug 914220 because the patches here are rebased on top of those, not because of a logical dependency.
Depends on: 914220
Attachment #802363 - Flags: review?(till)
Attachment #802365 - Flags: review?(till)
Comment on attachment 802361 [details] [diff] [review] Bug898362-Part1-ExposeConstants.diff Review of attachment 802361 [details] [diff] [review]: ----------------------------------------------------------------- For my sanity's sake, I mostly just skimmed the changes in TypedObject.cpp. I gather the vast majority of them would just cause compiler errors if anything went wrong. ::: js/src/builtin/TypedObjectConstants.h @@ +56,5 @@ > +#define JS_TYPEREPR_SLOTS 4 > + > +// These constants are for use exclusively in JS code. In C++ code, > +// prefer TypeRepresentation::Scalar etc, since that allows you to > +// write a switch which will receive a warning if you omit a case. s/receive/cause/? @@ +63,5 @@ > +#define JS_TYPEREPR_ARRAY_KIND 2 > + > +// These constants are for use exclusively in JS code. In C++ code, > +// prefer ScalarTypeRepresentation::TYPE_INT8 etc, since that allows > +// you to write a switch which will receive a warning if you omit a and here @@ +79,5 @@ > +/////////////////////////////////////////////////////////////////////////// > +// Slots for typed objects > + > +#define JS_TYPEDOBJ_SLOT_TYPE_OBJ 0 // Type object for a given typed object > +#define JS_TYPEDOBJ_SLOT_OWNER 2 // Owner of data (if null, this is owner) Why no slot 1?
Attachment #802361 - Flags: review?(till) → review+
Comment on attachment 802363 [details] [diff] [review] Bug898362-Part2-SelfHost.diff Review of attachment 802363 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty nice! ::: js/src/builtin/TypedObject.cpp @@ +197,5 @@ > return atom->asPropertyName(); > } > > +static JSFunction * > +SelfHostedFunction(JSContext *cx, const char *name) This should really take a PropertyName, with most or all of the names added to the atom table. Re-atomizing the names every time they're used will probably have meaningful overhead. Also, now that I think about it, would you mind moving this into SelfHosting and adding the declaration to jscntxt.h (or adding a SelfHosting.h, perhaps)? This would be useful in more places right now (I know the Intl stuff could use it) and will become more so over time. @@ +217,3 @@ > /* > + * Overwrites the contents of `block` at offset `offset` with `val` > + * converted to the type `typeObj` Full stop at the end, please. @@ +1750,5 @@ > + cx, > + &type->getReservedSlot(JS_TYPEOBJ_SLOT_STRUCT_FIELD_TYPES).toObject()); > + RootedValue fieldTypeVal(cx); > + if (!JSObject::getElement(cx, fieldTypes, fieldTypes, > + fieldIndex, &fieldTypeVal)) Either don't wrap the if, or put braces around the then. ::: js/src/builtin/TypedObject.h @@ +219,5 @@ > +// size) > +// > +// Intrinsic function. Copies size bytes from the data for > +// `sourceTypedObj` at `sourceOffset` into the data for > +// `targetTypedObj` at `targetOffset`. Nit: please keep the in-file style for function documentation. I.e. /*\n * comment\n */ @@ +228,5 @@ > +// Usage: StoreScalar(targetTypedObj, targetOffset, value) > +// > +// Intrinsic function. Stores value (which must be an int32 or uint32) > +// by `scalarTypeRepr` (which must be a type repr obj) and stores the > +// value at the memory for `targetTypedObj` at offset `targetOffset`. Same here ::: js/src/builtin/TypedObject.js @@ +48,5 @@ > +// > +// Most `TypedObjectPointers` methods are written in a "chaining" > +// style, meaning that they return `this`. This is true even though > +// they mutate the receiver in place, because it makes for prettier > +// code. While I generally don't care too much about comment style, this one threw me off. It makes the comment look like it introduces a new section of code, not an individual function. Maybe lose the first line of many "/"s and remove the empty line between the comment and the function? @@ +57,5 @@ > + this.owner = owner; > + this.offset = offset; > +} > + > +MakeConstructible(TypedObjectPointer, {}); Not that it matters too much, but I'd probably initialize the prototype like so: MakeConstructible(TypedObjectPointer, { copy: function[..], reset: function[..], kind: function[..], #ifdef DEBUG toString: function[..], #endif [..] }); @@ +86,5 @@ > +/////////////////////////////////////////////////////////////////////////// > +// moveTo(propName) > +// > +// Adjusts `this` in place so that it points at the property > +// `propName`. Throws if there is no such property. Returns `this`. See my comment on comment style above. @@ +160,5 @@ > +// set(fromValue) > +// > +// Assigns `fromValue` to the memory pointed at by `this`, adapting it > +// to `typeRepr` as needed. This is the most general entry point and > +// works for any type. See my comment on comment style above. @@ +215,5 @@ > + } > + > + ThrowError(JSMSG_CANT_CONVERT_TO, > + typeof(fromValue), > + this.typeRepr.toSource()) No need to wrap this, plus missing semicolon. @@ +263,5 @@ > +function ConvertAndCopyTo(destTypeRepr, > + destTypeObj, > + destTypedObj, > + destOffset, > + fromValue) { Brace on new line, please.
Attachment #802363 - Flags: review?(till) → review+
Attachment #802365 - Flags: review?(till) → review+
Blocks: 914137
Blocks: 926401
Attached patch Bug898362.diffSplinter Review
Carrying over r+ from till
Attachment #802361 - Attachment is obsolete: true
Attachment #802363 - Attachment is obsolete: true
Attachment #802365 - Attachment is obsolete: true
Attachment #819165 - Flags: review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: