Summary: | Use the Windows thread pool instead of an extra thread for FastMalloc scavenging | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Roben (:aroben) <aroben> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, aroben, commit-queue, eric, jberlin, paroga, webkit.review.bot | ||||||||||||||
Priority: | P2 | Keywords: | InRadar, PlatformOnly | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | PC | ||||||||||||||||
OS: | Windows XP | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 52055 | ||||||||||||||||
Attachments: |
|
Description
Adam Roben (:aroben)
2010-09-03 11:49:40 PDT
Created attachment 78526 [details]
Patch
Attachment 78526 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1442: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1444: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1506: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1550: TCMalloc_PageHeap::scavengerThread is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2436: TCMalloc_PageHeap::runScavenge is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 5 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 78526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78526&action=review > Source/JavaScriptCore/wtf/FastMalloc.cpp:1566 > + while (1) { > + if (runScavenge()) > + continue; > + > + pthread_mutex_lock(&m_scavengeMutex); > + m_scavengeThreadActive = false; > + // Block until there are enough free committed pages to release back to the system. > + pthread_cond_wait(&m_scavengeCondition, &m_scavengeMutex); > + m_scavengeThreadActive = true; > + pthread_mutex_unlock(&m_scavengeMutex); > + } I think it would be a lot better if we followed the libdispatch implementation. periodicScavenge just performs a single scavenge, then waits for the timer to fire again (or stops the timer if no more scavenging is needed). That way we don't tie up a worker thread while we sleep and block. Comment on attachment 78526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78526&action=review >> Source/JavaScriptCore/wtf/FastMalloc.cpp:1566 >> + } > > I think it would be a lot better if we followed the libdispatch implementation. periodicScavenge just performs a single scavenge, then waits for the timer to fire again (or stops the timer if no more scavenging is needed). That way we don't tie up a worker thread while we sleep and block. Patrick pointed out that I was misreading the patch and that this code isn't executing on Windows anymore. However, runScavenge still includes a sleep() call, which I don't think we should be doing on the worker thread. We should use a timer queue timer so that the waiting happens on a thread pool wait thread. Then when the timer fires, we should just scavenge immediately. Created attachment 78538 [details]
Patch
Attachment 78538 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1442: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1446: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1447: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1448: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1450: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1498: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1500: Use 0 instead of NULL. [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1504: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1504: Missing space before { [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1524: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2408: TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 11 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 78538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78538&action=review It's really too bad we can't reuse a previously-paused timer queue timer! > Source/JavaScriptCore/wtf/FastMalloc.cpp:1536 > +ALWAYS_INLINE void TCMalloc_PageHeap::signalScavenger() > +{ > + ASSERT(IsHeld(pageheap_lock)); > + if (!m_scavengingScheduled && shouldScavenge()) { > + m_scavengingScheduled = true; > + dispatch_resume(m_scavengeTimer); > + } > +} > + > +#elif OS(WINDOWS) > + > +static void CALLBACK scavengerTimerFired(void* context, BOOLEAN) > +{ > + static_cast<TCMalloc_PageHeap*>(context)->periodicScavenge(); > +} > + > +void TCMalloc_PageHeap::initializeScavenger() > +{ > + m_scavengeQueueTimer = 0; > +} > + > +ALWAYS_INLINE void TCMalloc_PageHeap::signalScavenger() > +{ > + ASSERT(IsHeld(pageheap_lock)); > + if (!m_scavengeQueueTimer && shouldScavenge()) { > + DWORD period = kScavengeDelayInSeconds * 1000; > + CreateTimerQueueTimer(&m_scavengeQueueTimer, 0, scavengerTimerFired, 0, period, period, WT_EXECUTEDEFAULT); > + } > +} It would be nice if we could share a little more of the logic in signalScavenger(). Adding some new functions like isScavengerScheduled() (or its opposite, isScavengerSuspended()) and scheduleScavenger() should make that pretty easy. > Source/JavaScriptCore/wtf/FastMalloc.cpp:2421 > +#if HAVE(DISPATCH_H) > + m_scavengingScheduled = false; > + dispatch_suspend(m_scavengeTimer); > +#elif OS(WINDOWS) > + HANDLE scavengeQueueTimer = m_scavengeQueueTimer; > + m_scavengeQueueTimer = 0; > + DeleteTimerQueueTimer(0, scavengeQueueTimer, 0); > #endif Maybe a suspendScavenger() function would make this cleaner. Comment on attachment 78538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78538&action=review >> Source/JavaScriptCore/wtf/FastMalloc.cpp:1536 >> +} > > It would be nice if we could share a little more of the logic in signalScavenger(). Adding some new functions like isScavengerScheduled() (or its opposite, isScavengerSuspended()) and scheduleScavenger() should make that pretty easy. I don't think there's anything that prevents m_scavengeQueueTimer from firing again if periodicScavenge is taking a long time. Maybe we should always use a non-repeating timer and delete it every time it fires? Created attachment 78976 [details]
Patch
Attachment 78976 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1450: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512: Use 0 instead of NULL. [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: Missing space before { [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1549: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2447: TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 15 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 78976 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78976&action=review > Source/JavaScriptCore/wtf/FastMalloc.cpp:1540 > +ALWAYS_INLINE bool TCMalloc_PageHeap::isScavengerSuspended() > +{ > + return m_scavengingSuspended; > +} > +ALWAYS_INLINE void TCMalloc_PageHeap::scheduleScavenger() > +{ > + m_scavengingSuspended = false; > + dispatch_resume(m_scavengeTimer); > +} > + > +ALWAYS_INLINE void TCMalloc_PageHeap::rescheduleScavenger() > +{ > + // Nothing to do here for libdispatch. > +} > + > +ALWAYS_INLINE void TCMalloc_PageHeap::suspendScavenger() > +{ > + m_scavengingSuspended = true; > + dispatch_suspend(m_scavengeTimer); > +} These functions (and their Windows equivalents) should probably assert that the pageheap lock is held. > Source/JavaScriptCore/wtf/FastMalloc.cpp:1561 > + CreateTimerQueueTimer(&m_scavengeQueueTimer, 0, scavengerTimerFired, 0, kScavengeDelayInSeconds * 1000, 0, WT_EXECUTEONLYONCE); It would be helpful to add a comment here about how rescheduling a non-repeating timer doesn't work. > Source/JavaScriptCore/wtf/FastMalloc.cpp:2459 > + { > + SpinLockHolder h(&pageheap_lock); > + pageheap->scavenge(); > + } > + > + if (shouldScavenge()) { > + suspendScavenger(); > + return; > + } > + > + rescheduleScavenger(); This doesn't look right. I think we need to hold pageheap_lock when calling the various *scavenge* functions. Created attachment 78983 [details]
Patch
Attachment 78983 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1450: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512: Use 0 instead of NULL. [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: Missing space before { [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1552: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2455: TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 15 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 78983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78983&action=review > Source/JavaScriptCore/wtf/FastMalloc.cpp:1566 > + // We need to use WT_EXECUTEONLYONCE here and reschedule the timer, because > + // Windows will fire the timer event even when the function is already running. Passing 0 for the repeat duration should be enough to make the timer fire only once. If you pass WT_EXECUTEDEFAULT, are you then able to use ChangeTimerQueueTimer to reuse the same timer? If that still doesn't work, I think it's worth adding a comment in rescheduleScavenger and/or suspendScavenger that explains why we have to delete and recreate the timer. > Source/JavaScriptCore/wtf/FastMalloc.cpp:2463 > + if (shouldScavenge()) { > + suspendScavenger(); > + return; > + } I think you're missing a ! in there. Created attachment 78987 [details]
Patch
Attachment 78987 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/wtf/FastMalloc.cpp:1450: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1451: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1452: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1453: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1454: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1458: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1459: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1460: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1462: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1510: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1512: Use 0 instead of NULL. [readability/null] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: More than one command on the same line [whitespace/newline] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1516: Missing space before { [whitespace/braces] [5]
Source/JavaScriptCore/wtf/FastMalloc.cpp:1552: TCMalloc_PageHeap::initializeScavenger is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Source/JavaScriptCore/wtf/FastMalloc.cpp:2456: TCMalloc_PageHeap::periodicScavenge is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 15 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 78987 [details] Patch Rejecting attachment 78987 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build'..." exit_code: 2 Last 500 characters of output: se/testapi.build/Script-14D857B50A469C100032146C.sh === BUILD AGGREGATE TARGET All OF PROJECT JavaScriptCore WITH CONFIGURATION Release === Check dependencies ** BUILD FAILED ** The following build commands failed: JavaScriptCore: CompileC /mnt/git/webkit-commit-queue/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/FastMalloc.o /mnt/git/webkit-commit-queue/Source/JavaScriptCore/wtf/FastMalloc.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2 (1 failure) Full output: http://queues.webkit.org/results/7558051 Created attachment 78995 [details]
Patch
Comment on attachment 78995 [details] Patch Clearing flags on attachment: 78995 Committed r75819: <http://trac.webkit.org/changeset/75819> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build) (In reply to comment #22) > http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build) I think this change might have broken the Windows 7 Release Tests: http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8236 http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8237 (In reply to comment #23) > (In reply to comment #22) > > http://trac.webkit.org/changeset/75819 might have broken Windows Release (Build) > > I think this change might have broken the Windows 7 Release Tests: > > http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8236 > http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/8237 It did, but Patrick fixed it in subsequent checkins. Thanks for watching the bots! |