Closed
Bug 801839
Opened 12 years ago
Closed 12 years ago
IonMonkey: Clean up jsop_getprop().
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: sstangl, Unassigned)
Details
(Whiteboard: [ion:t])
Attachments
(1 file)
16.42 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
I've been complaining about jsop_getprop() for a while. This finally cleans it up.
Attachment #671587 -
Flags: review?(nicolas.b.pierron)
Comment 1•12 years ago
|
||
Comment on attachment 671587 [details] [diff] [review]
patch
Review of attachment 671587 [details] [diff] [review]:
-----------------------------------------------------------------
Nice refactoring moving complex if statements in well named functions which can easily be commented or sorted differently. The emitted pattern is a bit difficult to read at first but is convenient because we can reuse the abort function for the return value of any sub-functions.
The only trouble that need to be addressed during reviews is that no “return true;” is added after any MIR allocation or graph modification.
Most of our optimizations phase can be rewritten with this “emitted pattern” instead of using a 3 state enum return, such as inlining of native functions.
::: js/src/ion/IonBuilder.cpp
@@ +5801,5 @@
> +
> + // Try to hardcode known constants.
> + if (!GetPropTryConstant(&emitted, obj, id) || emitted)
> + return emitted;
> +
nit: trailing white space
@@ +5833,5 @@
> + return pushTypeBarrier(call, types, barrier);
> +}
> +
> +bool
> +IonBuilder::GetPropTryArgumentsLength(bool *emitted)
nit: follow the convention used for IonBuilder. The first upper case letter is used for static functions.
@@ +5835,5 @@
> +
> +bool
> +IonBuilder::GetPropTryArgumentsLength(bool *emitted)
> +{
> + *emitted = false;
nit: emitted should always be false, no need to reset it. Otherwise assert that it is still false.
@@ +5843,5 @@
> return abort("Type is not definitely lazy arguments.");
> + if (isArguments != DefinitelyArguments)
> + return true;
> +
> + if (JSOp(*pc) == JSOP_LENGTH) {
nit: Invert the condition and remove the braces.
@@ +5903,4 @@
>
> + TypeOracle::Unary unary = oracle->unaryOp(script_, pc);
> + TypeOracle::UnaryTypes unaryTypes = oracle->unaryTypes(script_, pc);
> + types::StackTypeSet *barrier = oracle->propertyReadBarrier(script_, pc);
Make sure that TI is not freezing twice.
@@ +5946,2 @@
> if (!TestCommonPropFunc(cx, unaryTypes.inTypes, id, &commonGetter, true, &isDOM))
> + return false;
This TestCommonPropFunc if-statement sounds weird or badly named, it sounds like the commonGetter flag behaves in a similar way as the emitted flag. Can you re-use the same pattern for this function.
@@ +6036,5 @@
> + // The input value must either be an object, or we should have strong suspicions
> + // that it can be safely unboxed to an object.
> + if (unary.ival != MIRType_Object && !unaryTypes.inTypes->objectOrSentinel())
> + return true;
> +
nit: trailing white-space.
@@ +6073,3 @@
>
> + if (oracle->propertyReadAccessGetter(script_, pc))
> + monitorResult(load, barrier, types);
nit: Reuse the variable “accessGetter” in the condition.
Attachment #671587 -
Flags: review?(nicolas.b.pierron) → review+
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/55d64487e54a
Backed out: M1 failures.
Reporter | ||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•