Closed Bug 512645 Opened 15 years ago Closed 15 years ago

Only clamp nested timeouts

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: vlad, Assigned: peterv)

References

Details

(Keywords: perf, Whiteboard: [ts])

Attachments

(1 file)

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
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.
Attached patch v1 (deleted) — Splinter Review
Still trying to write a testcase for this, but it's not easy to make a reliable one.
Attachment #402351 - Flags: review?(jst)
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+
Keywords: perf
Whiteboard: [ts] → [ts][has patch][can land]
Sorry for the delay in checking this in. http://hg.mozilla.org/mozilla-central/rev/350ffc1d793a
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [ts][has patch][can land] → [ts]
Target Milestone: --- → mozilla1.9.3a1
Depends on: 531542
Depends on: 597644
No longer depends on: 597644
No longer regressions: 1646799
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: