Closed Bug 892634 Opened 9 years ago Closed 8 years ago

PJS: Supporting ToNumber and bit ops on values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: shu, Assigned: shu)

References

Details

Attachments

(4 files, 2 obsolete files)

We'd like to support bit ops on values, which would involve making threadsafe versions of the various ToNumber functions.
Attached patch WIP: Refactoring for code size (obsolete) — Splinter Review
Duplicating all the ToInt stuff would be a shame since most of the code will be identical.

Here's a WIP that favors refactoring some VM functions to use branches on the context instead of templates.

I'm planning for the following trichotomy of VM functions:

 - Thread safe. These take ThreadSafeContext and returns bool. e.g., ConcatStrings in this patch.
 - Not thread safe. These take JSContext and returns bool.
 - Thread safe given slight behavior change. These take ThreadSafeContext and return ParallelResult. Thin wrappers can be given for the sequential case that take JSContext and returns bool by comparing the ParallelResult == TP_SUCCESS.

Feedback welcome!

P.S. Might want to change the name ParallelResult going forward.
Assignee: general → shu
Attachment #774171 - Flags: feedback?(wmccloskey)
Attachment #774171 - Flags: feedback?(bhackett1024)
Small clarification: "slight behavior change" above means slight observable, non-timing behavior change, like the case of ToInt32 in the patch, where we fail with TP_RETRY_SEQUENTIALLY for objects if !cx->isJSContext().
Comment on attachment 774171 [details] [diff] [review]
WIP: Refactoring for code size

Review of attachment 774171 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this seems okay to me.

::: js/src/jsnum.cpp
@@ +1413,5 @@
> +
> +bool
> +js::StringToNumber(ThreadSafeContext *cx, JSString *str, double *result)
> +{
> +    if (cx->isJSContext()) {

Could you take out the isJSContext() case and just use ScopedThreadSafeStringInspector for everything? It seems like it does pretty much the same thing as far as I can tell.

@@ +1433,5 @@
>  # pragma optimize("g", off)
>  #endif
>  
> +ParallelResult
> +js::ToNumberSlow(ThreadSafeContext *cx, Value v, double *out)

Having this return ParallelResult seems weird to me. Could we split apart the code that doesn't deal with objects into a separate function? PJS could do its own isObject check and then call that.

::: js/src/vm/String.cpp
@@ +429,4 @@
>  
>          jschar *buf = str->init(wholeLength);
> +
> +        if (cx->isJSContext()) {

Same thing here about just using ScopedThreadSafeStringInspector for everything.
Attachment #774171 - Flags: feedback?(wmccloskey) → feedback+
Depends on: 895950
Attachment #774171 - Attachment is obsolete: true
Attachment #774171 - Flags: feedback?(bhackett1024)
Attachment #779242 - Flags: review?(wmccloskey)
Attachment #779243 - Flags: review?(nmatsakis)
This part depends on bug 895950.
Attachment #779247 - Flags: review?(jdemooij)
Fix an issue with LValueToInt32::mir() assuming the MIR is always MToInt32.
Attachment #779247 - Attachment is obsolete: true
Attachment #779247 - Flags: review?(jdemooij)
Attachment #779275 - Flags: review?(jdemooij)
Attached patch Part 4: jit-testSplinter Review
r=me for test
Attachment #779276 - Flags: review+
Attachment #779275 - Flags: review?(jdemooij) → review+
Attachment #779243 - Flags: review?(nmatsakis) → review+
Comment on attachment 779242 [details] [diff] [review]
Part 1: Unify ConcatStrings and add some threadsafe nonobject-to-number-type conversion fns

Review of attachment 779242 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.cpp
@@ +1435,5 @@
> +
> +    if (v.isString())
> +        return StringToNumber(cx, v.toString(), out);
> +    if (v.isBoolean()) {
> +        if (v.toBoolean()) {

This would probably work better using the ternary operator:
  *out = v.toBoolean() ? 1.0 : 0.0;

@@ +1491,5 @@
> +         * On the second iteration of the loop, we would have already checked
> +         * the new v is not an object.
> +         */
> +        JS_ASSERT(!v.isObject());
> +        goto skip_object;

I don't think it's worth it to skip this check. We have enough gotos here already.

@@ -1478,5 @@
> -        if (v.isUndefined())
> -            break;
> -
> -        JS_ASSERT(v.isObject());
> -        if (!cx->isJSContext())

I think we still want this isJSContext() check.

::: js/src/jsnum.h
@@ +284,5 @@
> +
> +bool
> +NonObjectToNumberSlow(ThreadSafeContext *cx, const Value &v, double *out);
> +
> +JS_ALWAYS_INLINE bool

Unless there's a good reason for it, it's better to avoid JS_ALWAYS_INLINE. We have way too much of it already.

@@ +287,5 @@
> +
> +JS_ALWAYS_INLINE bool
> +NonObjectToNumber(ThreadSafeContext *cx, const Value &v, double *out)
> +{
> +    if (!v.isNumber()) {

I think this is reversed.
Attachment #779242 - Flags: review?(wmccloskey) → review+
You need to log in before you can comment on or make changes to this bug.