Closed Bug 1128732 Opened 5 years ago Closed 5 years ago

Simplify js::DefineProperties

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(1 file, 1 obsolete file)

Somehow this got all crufty. Easily fixed.
/r/3305 - Bug 1128732 - Simplify js::DefineProperties. r?efaust.

Pull down this commit:

hg pull review -r df69ea5733b6c288060e01c0b0ea1fb8947ab8f4
Attachment #8558884 - Flags: review?(efaustbmo)
This reverses changes that happened in bug 885729. In my view, those changes were a mistake for a couple of reasons. First, we probably should never take patches that add code solely for performance reasons without at least crude measurements. (I admit I occasionally get the temptation myself.) At least we should be strongly prejudiced against taking such patches.

In this particular case, I also don't imagine that patch was all that effective, because:

1.  ReadPropertyDescriptors probably dominates the runtime.

2.  Object.defineProperties on an array is probably vanishingly rare, even in the future.

3.  The speed of Object.defineProperties on a proxy probably wasn't measurably improved by the patch, given that it only removes a branch or two and an indirect function call per property defined. Compared to the proxy overhead still incurred, that's a pretty tiny win.
https://reviewboard.mozilla.org/r/3305/#review2715

Yeah, no need to pull special cases on top of special cases...
https://hg.mozilla.org/mozilla-central/rev/935446ec6fb9
Assignee: nobody → jorendorff
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment on attachment 8558884 [details]
MozReview Request: bz://1128732/jorendorff

Reviewboard snafu. This was already reviewed and landed.
Attachment #8558884 - Flags: review?(efaustbmo) → review+
Attachment #8558884 - Attachment is obsolete: true
Attachment #8619307 - Flags: review+
You need to log in before you can comment on or make changes to this bug.