Only clamp nested timeouts

RESOLVED FIXED in mozilla1.9.3a1

Status

()

defect
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: vlad, Assigned: peterv)

Tracking

({perf})

unspecified
mozilla1.9.3a1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ts])

Attachments

(1 attachment)

chrome uses setTimeout(0) often to just wait for an event loop cycle or similar; we should not enforce the 10ms minimum for chrome windows.
Summary: setTimeout sohuld not enforce DOM_MIN_TIMEOUT_VALUE for chrome → setTimeout should not enforce DOM_MIN_TIMEOUT_VALUE for chrome
(Assignee)

Comment 2

10 years ago
I'm going to morph this, we just discussed this yesterday. We shouldn't special-case chrome, we should follow HTML5 and only clamp for nested timeouts. HTML5 says to clamp for nesting level >= 1, Webkit seems to clamp for nesting level >= 5. iBench chains 3 timeouts, so following Webkit would also help our score there.
Assignee: nobody → peterv
Summary: setTimeout should not enforce DOM_MIN_TIMEOUT_VALUE for chrome → Only clamp nested timeouts
Whatever we do here, we should make sure to tweak the worker setTimeout code as well as the normal Window one.
(Assignee)

Comment 4

10 years ago
Posted patch v1Splinter Review
Still trying to write a testcase for this, but it's not easy to make a reliable one.
Attachment #402351 - Flags: review?(jst)
(Assignee)

Comment 5

10 years ago
I copied WebKit for now, using 5 for the nesting level at which to start clamping.

(In reply to comment #3)
> Whatever we do here, we should make sure to tweak the worker setTimeout code as
> well as the normal Window one.

Workers don't seem to clamp timeouts at all. Want to file a separate bug for adding clamping?
Actually, would it make sense to not do what webkit does and instead simply follow the spec, and then point to this as for why iBench is totally broken in its current state?
(In reply to comment #7)
> Filed bug 518406 on worker timeouts.

Punks! ;)
(In reply to comment #6)
> Actually, would it make sense to not do what webkit does and instead simply
> follow the spec, and then point to this as for why iBench is totally broken in
> its current state?

I don't think it would.

I don't see how allowing some nesting this way is worse for the web, since spec-compliant code still runs fine, and complaining about iBench is unlikely to have any effect on the world (it'll never get changed).
I agree iBench is unlikely to get fixed. And that this fix isn't bad for the web.

However my understanding was that it was so broken that it's unlikely that we'll ever get any useful data out of it, so we'd really like Apple to quit using it. Having glaring bugs like this to point at would give us a strong case for that Apple is not acting in good faith if they keep using it.
iBench has the bug regardless of what we do here, so point away!
are there open questions here or just waiting on review? if the broadening of the change suggested in comment #0 is slowing things down, can we just do the minimal change first?
I think we're just waiting for jsts review.
Attachment #402351 - Flags: superreview+
Attachment #402351 - Flags: review?(jst)
Attachment #402351 - Flags: review+

Updated

10 years ago
Keywords: perf
Whiteboard: [ts] → [ts][has patch][can land]
(Assignee)

Comment 14

10 years ago
Sorry for the delay in checking this in.

http://hg.mozilla.org/mozilla-central/rev/350ffc1d793a
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [ts][has patch][can land] → [ts]
Target Milestone: --- → mozilla1.9.3a1

Updated

10 years ago
Depends on: 531542

Updated

9 years ago
Depends on: 597644
No longer depends on: 597644
You need to log in before you can comment on or make changes to this bug.