28455 2009-08-19 08:37:29 -0700 ThreadTimer: avoid blocking UI when too many timers ready to fire 2009-08-21 00:16:08 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Other Other RESOLVED DUPLICATE 23865 P2 Normal --- 1 yong.li.webkit webkit-unassigned darin dimich eric staikos oldest_to_newest 141479 0 yong.li.webkit 2009-08-19 08:37:29 -0700 Current ThreadTimer fires all timers that meet the fire time. This can block UI, if some of those timers are big time-consumers. Solution is to set a timeout when firing those timers. When time is out, those unfired timers should be postponed to next time. 141483 1 35123 yong.li.webkit 2009-08-19 08:51:12 -0700 Created attachment 35123 the patch 141525 2 eric 2009-08-19 10:44:59 -0700 Darin Adler is probably your best bet for review here. 141538 3 35123 darin 2009-08-19 11:24:35 -0700 Comment on attachment 35123 the patch The concept of the code is here OK, but the conditionals need to be done better. I think you should have the TimerBeingFired struct all the time, and just make the fire time member be conditional. Normally we do not use the m_ prefix for struct members, only for classes where they are private. > + for (Vector<TimerBeingFired>::const_iterator i = firingTimers.begin(); i != firingTimers.end(); ++i) { > + TimerBase* timer = i->m_timer; > for (size_t i = 0; i != size; ++i) { > TimerBase* timer = firingTimers[i]; We do not normally use iterators to go through a Vector. Instead we just use indexing as you would with an array. That's what we did in the existing code, and there's no reason to change that just because the timers are a struct. > + // 30ms > + if (currentTime() - startTime > 0.03) { The time interval should be a constant at the top of the file, and the comment should explain why that particular interval was chosen. > + for (++i; i != firingTimers.end(); ++i) { > + timer = i->m_timer; > + if (timer->m_nextFireTime == 0 && m_timersReadyToFire.contains(timer)) > + timer->setNextFireTime(i->m_fireTime); > + } I wrote the class, and yet I do not understand this code. It may be correct. There needs to be a comment explaining why this is the right thing to do. 141550 4 dimich 2009-08-19 12:43:42 -0700 I refactored the code Darin originally wrote to the point when I almost understand it :-) so let me add a comment: I think this will remove and then re-insert unfired timers into the heap structure, causing unneeded sort operations - which might be unwanted in a scenario when we have so many timers it freezes UI... Bug 23865 has a patch addressing the same very issue, but instead of taking all the ready timers first into the separate list and then re-inserting the unfired timers back, it just fires them in a loop one-by-one, taking them from original heap until time interval permits. This avoids taking and then re-inserting timers into timer heap, while still keeping track of removed and rescheduled timers. That patch was not landed because the original bug was about even bigger issue (recursive multiplication of timers) - but for avoiding UI freeze IMHO the patch would work better. Also, I think it is useful for any port and doesn't affect functionality so it doesn't need ENABLE define guards. If you find this makes sense, please feel free to use the code in Bug 23865 as an idea or ping me to continue to work on it... 141590 5 yong.li.webkit 2009-08-19 14:11:02 -0700 > > + for (Vector<TimerBeingFired>::const_iterator i = firingTimers.begin(); i != firingTimers.end(); ++i) { > > + TimerBase* timer = i->m_timer; > > > for (size_t i = 0; i != size; ++i) { > > TimerBase* timer = firingTimers[i]; > > We do not normally use iterators to go through a Vector. Instead we just use > indexing as you would with an array. That's what we did in the existing code, > and there's no reason to change that just because the timers are a struct. I agree with the rest of your suggestions. but for this one I think using iterator (or const_iterator) should be better. First, "iterator" is kind of standard way for enumerating a C++ container. typedef Vector<some type> MyContainerType; for (MyContainerType::const_iterator i = container.begin(); i != container.end(); ++i) { } If someday you change the container type from Vector to List, you don't have to change the code that enumerates the container. Second, I think the fastest way of walking through an array is using an increasing pointer instead of an index, because array[i] may require i * sizeof(T) to get the address of the element. Although some compilers can be smart enough to optimize this, I saw replacing array index with increasing pointer boosted performance in practice. Vector<T>::iterator is just a pointer T*, and so that it doesn't rely on how smart the compiler is to make the code run faster. 141599 6 yong.li.webkit 2009-08-19 14:21:40 -0700 (In reply to comment #4) > I refactored the code Darin originally wrote to the point when I almost > understand it :-) so let me add a comment: I think this will remove and then > re-insert unfired timers into the heap structure, causing unneeded sort > operations - which might be unwanted in a scenario when we have so many timers > it freezes UI... > > Bug 23865 has a patch addressing the same very issue, but instead of taking all > the ready timers first into the separate list and then re-inserting the unfired > timers back, it just fires them in a loop one-by-one, taking them from original > heap until time interval permits. This avoids taking and then re-inserting > timers into timer heap, while still keeping track of removed and rescheduled > timers. > > That patch was not landed because the original bug was about even bigger issue > (recursive multiplication of timers) - but for avoiding UI freeze IMHO the > patch would work better. Also, I think it is useful for any port and doesn't > affect functionality so it doesn't need ENABLE define guards. > > If you find this makes sense, please feel free to use the code in Bug 23865 as > an idea or ping me to continue to work on it... If that patch doesn't (In reply to comment #4) > I refactored the code Darin originally wrote to the point when I almost > understand it :-) so let me add a comment: I think this will remove and then > re-insert unfired timers into the heap structure, causing unneeded sort > operations - which might be unwanted in a scenario when we have so many timers > it freezes UI... > > Bug 23865 has a patch addressing the same very issue, but instead of taking all > the ready timers first into the separate list and then re-inserting the unfired > timers back, it just fires them in a loop one-by-one, taking them from original > heap until time interval permits. This avoids taking and then re-inserting > timers into timer heap, while still keeping track of removed and rescheduled > timers. > > That patch was not landed because the original bug was about even bigger issue > (recursive multiplication of timers) - but for avoiding UI freeze IMHO the > patch would work better. Also, I think it is useful for any port and doesn't > affect functionality so it doesn't need ENABLE define guards. > > If you find this makes sense, please feel free to use the code in Bug 23865 as > an idea or ping me to continue to work on it... Hm... It seems better. 141656 7 darin 2009-08-19 17:23:31 -0700 > I agree with the rest of your suggestions. but for this one I think using > iterator (or const_iterator) should be better. > > First, "iterator" is kind of standard way for enumerating a C++ container. > > typedef Vector<some type> MyContainerType; > > for (MyContainerType::const_iterator i = container.begin(); i != > container.end(); ++i) { > } > > If someday you change the container type from Vector to List, you don't have to > change the code that enumerates the container. I don't agree. I am aware that using iterators lets you use the same code on Vector and List. I find the code using indices much easier to read and have never had an experience where I had to change code from Vector to List. I do find it useful for generic code that works on, say, both Vector and List. 141837 8 dimich 2009-08-21 00:16:08 -0700 Re-resolving due to Bugzilla data loss. The bug 23865 now has the patch fixing this issue. *** This bug has been marked as a duplicate of bug 23865 *** 35123 2009-08-19 08:51:12 -0700 2009-08-19 11:24:34 -0700 the patch 28455.patch text/plain 5166 yong.li.webkit ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg YTJhNWRlYy4uZWQwMzY2ZCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNiBAQAorMjAwOS0wOC0xOSAgWW9uZyBMaSAgPHlv bmcubGlAdG9yY2htb2JpbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEF2b2lkIHRpbWVyIGJsb2NraW5nIFVJIGJ5IHNldHRpbmcgYSAzMG1z IHRpbWUgb3V0CisgICAgICAgIHdoZW4gZmlyaW5nIHRpbWVycy4gVGhpcyBmZWF0dXJlIGNhbiBi ZSB0dXJuZWQKKyAgICAgICAgb24vb2ZmIGJ5IEVOQUJMRV9BVk9JRF9USU1FUl9CTE9DS0lOR19V SQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9Mjg0NTUK KworICAgICAgICAqIHBsYXRmb3JtL1RocmVhZFRpbWVycy5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpUaHJlYWRUaW1lcnM6OmNvbGxlY3RGaXJpbmdUaW1lcnMpOgorICAgICAgICAqIHBsYXRmb3Jt L1RocmVhZFRpbWVycy5oOgorCiAyMDA5LTA4LTE5ICBKYW4gTWljaGFlbCBBbG9uem8gIDxqbWFs b256b0B3ZWJraXQub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFhhbiBMb3Blei4KZGlmZiAt LWdpdCBhL1dlYkNvcmUvcGxhdGZvcm0vVGhyZWFkVGltZXJzLmNwcCBiL1dlYkNvcmUvcGxhdGZv cm0vVGhyZWFkVGltZXJzLmNwcAppbmRleCA3MWEwNmIwLi41MjlhNjkzIDEwMDY0NAotLS0gYS9X ZWJDb3JlL3BsYXRmb3JtL1RocmVhZFRpbWVycy5jcHAKKysrIGIvV2ViQ29yZS9wbGF0Zm9ybS9U aHJlYWRUaW1lcnMuY3BwCkBAIC0xLDYgKzEsNyBAQAogLyoKICAqIENvcHlyaWdodCAoQykgMjAw NiwgMjAwOCBBcHBsZSBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCiAgKiBDb3B5cmlnaHQgKEMp IDIwMDkgR29vZ2xlIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KKyAqIENvcHlyaWdodCAoQykg MjAwOSBUb3JjaCBNb2JpbGUsIEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KICAqCiAgKiBSZWRp c3RyaWJ1dGlvbiBhbmQgdXNlIGluIHNvdXJjZSBhbmQgYmluYXJ5IGZvcm1zLCB3aXRoIG9yIHdp dGhvdXQKICAqIG1vZGlmaWNhdGlvbiwgYXJlIHBlcm1pdHRlZCBwcm92aWRlZCB0aGF0IHRoZSBm b2xsb3dpbmcgY29uZGl0aW9ucwpAQCAtNzksMjMgKzgwLDM5IEBAIHZvaWQgVGhyZWFkVGltZXJz Ojp1cGRhdGVTaGFyZWRUaW1lcigpCiAgICAgICAgIG1fc2hhcmVkVGltZXItPnNldEZpcmVUaW1l KG1fdGltZXJIZWFwLmZpcnN0KCktPm1fbmV4dEZpcmVUaW1lKTsKIH0KIAotCisjaWYgRU5BQkxF KEFWT0lEX1RJTUVSX0JMT0NLSU5HX1VJKQordm9pZCBUaHJlYWRUaW1lcnM6OmNvbGxlY3RGaXJp bmdUaW1lcnMoZG91YmxlIGZpcmVUaW1lLCBWZWN0b3I8VGhyZWFkVGltZXJzOjpUaW1lckJlaW5n RmlyZWQ+JiBmaXJpbmdUaW1lcnMpCisjZWxzZQogdm9pZCBUaHJlYWRUaW1lcnM6OmNvbGxlY3RG aXJpbmdUaW1lcnMoZG91YmxlIGZpcmVUaW1lLCBWZWN0b3I8VGltZXJCYXNlKj4mIGZpcmluZ1Rp bWVycykKKyNlbmRpZgogewogICAgIHdoaWxlICghbV90aW1lckhlYXAuaXNFbXB0eSgpICYmIG1f dGltZXJIZWFwLmZpcnN0KCktPm1fbmV4dEZpcmVUaW1lIDw9IGZpcmVUaW1lKSB7CiAgICAgICAg IFRpbWVyQmFzZSogdGltZXIgPSBtX3RpbWVySGVhcC5maXJzdCgpOworI2lmIEVOQUJMRShBVk9J RF9USU1FUl9CTE9DS0lOR19VSSkKKyAgICAgICAgVGltZXJCZWluZ0ZpcmVkIHRpbWVyQmVpbmdG aXJlZCA9IHsgdGltZXIsIHRpbWVyLT5tX25leHRGaXJlVGltZSB9OworICAgICAgICBmaXJpbmdU aW1lcnMuYXBwZW5kKHRpbWVyQmVpbmdGaXJlZCk7CisjZWxzZQogICAgICAgICBmaXJpbmdUaW1l cnMuYXBwZW5kKHRpbWVyKTsKKyNlbmRpZgogICAgICAgICBtX3RpbWVyc1JlYWR5VG9GaXJlLmFk ZCh0aW1lcik7CiAgICAgICAgIHRpbWVyLT5tX25leHRGaXJlVGltZSA9IDA7CiAgICAgICAgIHRp bWVyLT5oZWFwRGVsZXRlTWluKCk7CiAgICAgfQogfQogCisjaWYgRU5BQkxFKEFWT0lEX1RJTUVS X0JMT0NLSU5HX1VJKQordm9pZCBUaHJlYWRUaW1lcnM6OmZpcmVUaW1lcnMoZG91YmxlIGZpcmVU aW1lLCBjb25zdCBWZWN0b3I8VGhyZWFkVGltZXJzOjpUaW1lckJlaW5nRmlyZWQ+JiBmaXJpbmdU aW1lcnMpCit7CisgICAgZG91YmxlIHN0YXJ0VGltZSA9IGN1cnJlbnRUaW1lKCk7CisgICAgZm9y IChWZWN0b3I8VGltZXJCZWluZ0ZpcmVkPjo6Y29uc3RfaXRlcmF0b3IgaSA9IGZpcmluZ1RpbWVy cy5iZWdpbigpOyBpICE9IGZpcmluZ1RpbWVycy5lbmQoKTsgKytpKSB7CisgICAgICAgIFRpbWVy QmFzZSogdGltZXIgPSBpLT5tX3RpbWVyOworI2Vsc2UKIHZvaWQgVGhyZWFkVGltZXJzOjpmaXJl VGltZXJzKGRvdWJsZSBmaXJlVGltZSwgY29uc3QgVmVjdG9yPFRpbWVyQmFzZSo+JiBmaXJpbmdU aW1lcnMpCiB7CiAgICAgc2l6ZV90IHNpemUgPSBmaXJpbmdUaW1lcnMuc2l6ZSgpOwogICAgIGZv ciAoc2l6ZV90IGkgPSAwOyBpICE9IHNpemU7ICsraSkgewogICAgICAgICBUaW1lckJhc2UqIHRp bWVyID0gZmlyaW5nVGltZXJzW2ldOworI2VuZGlmCiAKICAgICAgICAgLy8gSWYgbm90IGluIHRo ZSBzZXQsIHRoaXMgdGltZXIgaGFzIGJlZW4gZGVsZXRlZCBvciByZS1zY2hlZHVsZWQgaW4gYW5v dGhlciB0aW1lcidzIGZpcmVkIGZ1bmN0aW9uLgogICAgICAgICAvLyBTbyBlaXRoZXIgd2UgZG9u J3Qgd2FudCB0byBmaXJlIGl0IGF0IGFsbCBvciB3ZSB3aWxsIGZpcmUgaXQgbmV4dCB0aW1lIHRo ZSBzaGFyZWQgdGltZXIgZ29lcyBvZmYuCkBAIC0xMTMsNiArMTMwLDE4IEBAIHZvaWQgVGhyZWFk VGltZXJzOjpmaXJlVGltZXJzKGRvdWJsZSBmaXJlVGltZSwgY29uc3QgVmVjdG9yPFRpbWVyQmFz ZSo+JiBmaXJpbmdUCiAgICAgICAgIC8vIENhdGNoIHRoZSBjYXNlIHdoZXJlIHRoZSB0aW1lciBh c2tlZCB0aW1lcnMgdG8gZmlyZSBpbiBhIG5lc3RlZCBldmVudCBsb29wLgogICAgICAgICBpZiAo IW1fZmlyaW5nVGltZXJzKQogICAgICAgICAgICAgYnJlYWs7CisKKyNpZiBFTkFCTEUoQVZPSURf VElNRVJfQkxPQ0tJTkdfVUkpCisgICAgICAgIC8vIDMwbXMKKyAgICAgICAgaWYgKGN1cnJlbnRU aW1lKCkgLSBzdGFydFRpbWUgPiAwLjAzKSB7CisgICAgICAgICAgICBmb3IgKCsraTsgaSAhPSBm aXJpbmdUaW1lcnMuZW5kKCk7ICsraSkgeworICAgICAgICAgICAgICAgIHRpbWVyID0gaS0+bV90 aW1lcjsKKyAgICAgICAgICAgICAgICBpZiAodGltZXItPm1fbmV4dEZpcmVUaW1lID09IDAgJiYg bV90aW1lcnNSZWFkeVRvRmlyZS5jb250YWlucyh0aW1lcikpCisgICAgICAgICAgICAgICAgICAg IHRpbWVyLT5zZXROZXh0RmlyZVRpbWUoaS0+bV9maXJlVGltZSk7CisgICAgICAgICAgICB9Cisg ICAgICAgICAgICBicmVhazsKKyAgICAgICAgfQorI2VuZGlmCiAgICAgfQogfQogCkBAIC0xMzAs NyArMTU5LDExIEBAIHZvaWQgVGhyZWFkVGltZXJzOjpzaGFyZWRUaW1lckZpcmVkSW50ZXJuYWwo KQogICAgIG1fZmlyaW5nVGltZXJzID0gdHJ1ZTsKIAogICAgIGRvdWJsZSBmaXJlVGltZSA9IGN1 cnJlbnRUaW1lKCk7CisjaWYgRU5BQkxFKEFWT0lEX1RJTUVSX0JMT0NLSU5HX1VJKQorICAgIFZl Y3RvcjxUaW1lckJlaW5nRmlyZWQ+ICBmaXJpbmdUaW1lcnM7CisjZWxzZQogICAgIFZlY3RvcjxU aW1lckJhc2UqPiBmaXJpbmdUaW1lcnM7CisjZW5kaWYKIAogICAgIC8vIG1fdGltZXJzUmVhZHlU b0ZpcmUgd2lsbCBpbml0aWFsbHkgY29udGFpbiB0aGUgc2FtZSBzZXQgYXMgZmlyaW5nVGltZXJz LCBidXQKICAgICAvLyBhcyB0aW1lcnMgZmlyZSBzb21lIG1hdCBiZWNvbWUgcmUtc2NoZWR1bGVk IG9yIGRlbGV0ZWQuIFRoZXkgZ2V0IHJlbW92ZWQgZnJvbQpkaWZmIC0tZ2l0IGEvV2ViQ29yZS9w bGF0Zm9ybS9UaHJlYWRUaW1lcnMuaCBiL1dlYkNvcmUvcGxhdGZvcm0vVGhyZWFkVGltZXJzLmgK aW5kZXggZWEwYTM2Ni4uOGIzMDQ3NyAxMDA2NDQKLS0tIGEvV2ViQ29yZS9wbGF0Zm9ybS9UaHJl YWRUaW1lcnMuaAorKysgYi9XZWJDb3JlL3BsYXRmb3JtL1RocmVhZFRpbWVycy5oCkBAIC0xLDYg KzEsNyBAQAogLyoKICAqIENvcHlyaWdodCAoQykgMjAwNiBBcHBsZSBDb21wdXRlciwgSW5jLiAg QWxsIHJpZ2h0cyByZXNlcnZlZC4KICAqIENvcHlyaWdodCAoQykgMjAwOSBHb29nbGUgSW5jLiAg QWxsIHJpZ2h0cyByZXNlcnZlZC4KKyAqIENvcHlyaWdodCAoQykgMjAwOSBUb3JjaCBNb2JpbGUs IEluYy4gQWxsIHJpZ2h0cyByZXNlcnZlZC4KICAqCiAgKiBSZWRpc3RyaWJ1dGlvbiBhbmQgdXNl IGluIHNvdXJjZSBhbmQgYmluYXJ5IGZvcm1zLCB3aXRoIG9yIHdpdGhvdXQKICAqIG1vZGlmaWNh dGlvbiwgYXJlIHBlcm1pdHRlZCBwcm92aWRlZCB0aGF0IHRoZSBmb2xsb3dpbmcgY29uZGl0aW9u cwpAQCAtNTMsOCArNTQsMTggQEAgbmFtZXNwYWNlIFdlYkNvcmUgewogICAgIHByaXZhdGU6CiAg ICAgICAgIHN0YXRpYyB2b2lkIHNoYXJlZFRpbWVyRmlyZWQoKTsKIAorI2lmIEVOQUJMRShBVk9J RF9USU1FUl9CTE9DS0lOR19VSSkKKyAgICAgICAgc3RydWN0IFRpbWVyQmVpbmdGaXJlZAorICAg ICAgICB7CisgICAgICAgICAgICBUaW1lckJhc2UqIG1fdGltZXI7CisgICAgICAgICAgICBkb3Vi bGUgbV9maXJlVGltZTsKKyAgICAgICAgfTsKKyAgICAgICAgdm9pZCBmaXJlVGltZXJzKGRvdWJs ZSBmaXJlVGltZSwgY29uc3QgVmVjdG9yPFRpbWVyQmVpbmdGaXJlZD4mKTsKKyAgICAgICAgdm9p ZCBjb2xsZWN0RmlyaW5nVGltZXJzKGRvdWJsZSBmaXJlVGltZSwgVmVjdG9yPFRpbWVyQmVpbmdG aXJlZD4mKTsKKyNlbHNlCiAgICAgICAgIHZvaWQgZmlyZVRpbWVycyhkb3VibGUgZmlyZVRpbWUs IGNvbnN0IFZlY3RvcjxUaW1lckJhc2UqPiYpOwogICAgICAgICB2b2lkIGNvbGxlY3RGaXJpbmdU aW1lcnMoZG91YmxlIGZpcmVUaW1lLCBWZWN0b3I8VGltZXJCYXNlKj4mKTsKKyNlbmRpZgogICAg ICAgICB2b2lkIHNoYXJlZFRpbWVyRmlyZWRJbnRlcm5hbCgpOwogICAgICAgICB2b2lkIGZpcmVU aW1lcnNJbk5lc3RlZEV2ZW50TG9vcEludGVybmFsKCk7CiAK