Closed
Bug 898362
Opened 11 years ago
Closed 11 years ago
[binary data] self-host where possible
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: nmatsakis, Assigned: nmatsakis)
References
Details
Attachments
(1 file, 3 obsolete files)
99.12 KB,
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Blocks: harmony:typedobjects
Assignee | ||
Comment 1•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: general → nmatsakis
Assignee | ||
Comment 2•11 years ago
|
||
Move ConvertAndCopyTo and binary fill into self-hosted code. More to come.
Assignee | ||
Comment 3•11 years ago
|
||
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.
Assignee | ||
Updated•11 years ago
|
Attachment #802361 -
Flags: review?(till)
Assignee | ||
Comment 4•11 years ago
|
||
Adding dependency on bug 914220 because the patches here are rebased on top of those, not because of a logical dependency.
Depends on: 914220
Assignee | ||
Updated•11 years ago
|
Attachment #802363 -
Flags: review?(till)
Assignee | ||
Updated•11 years ago
|
Attachment #802365 -
Flags: review?(till)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #802365 -
Flags: review?(till) → review+
Assignee | ||
Comment 7•11 years ago
|
||
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+
Assignee | ||
Comment 8•11 years ago
|
||
Clean try run: https://tbpl.mozilla.org/?tree=Try&rev=134073866259
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
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.
Description
•