1128732 - Simplify js::DefineProperties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Somehow this got all crufty. Easily fixed.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: bz://1128732/jorendorff

/r/3305 - Bug 1128732 - Simplify js::DefineProperties. r?efaust.

Pull down this commit:

hg pull review -r df69ea5733b6c288060e01c0b0ea1fb8947ab8f4

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Eric Faust [:efaust]]

https://reviewboard.mozilla.org/r/3305/#review2715

Yeah, no need to pull special cases on top of special cases...

Comment 4

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/935446ec6fb9

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/935446ec6fb9

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8558884 [details]
MozReview Request: bz://1128732/jorendorff

Reviewboard snafu. This was already reviewed and landed.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: MozReview Request: Bug 1128732 - Simplify js::DefineProperties. r?efaust.