Bug 45134 – Added statistics sampling and reporting for JavaScriptCore's RegisterFile and ExecutableAllocator classes

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[John Therrell]

In JavaScriptCore (cross-platform) :
Added thread-safe committed byte counting and reporting functionality to RegisterFile and ExecutableAllocator. Uses a static bool class member to provide runtime check if statistics collecting is enabled. Added Runtime checks do not impact performance on SunSpider benchmark. Allows for enabling and sampling statistics on release builds without conditional compilation.

In WebKit (mac only) :
Added ability to enable new JavaScriptCore statistics sampling and reporting for RegisterFile and ExecutableAllocator classes. Added reporting of JavaScriptCore's stack committed memory and JIT code committed memory statistics.

[John Therrell]

Created attachment 66415 [details]
Patch for added memory sampling/reporting for JavaScriptCore

[WebKit Review Bot]

Attachment 66415 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
spaces  [whitespace/tab] [1]
JavaScriptCore/interpreter/RegisterFile.h:203:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/interpreter/RegisterFile.h:204:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocator.h:300:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocator.h:301:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocator.h:304:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:131:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:132:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:138:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:139:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:140:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:345:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:346:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:423:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:424:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:425:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:425:  Missing space before {  [whitespace/braces] [5]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:426:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:427:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:440:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:441:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:443:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:449:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:453:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:454:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:456:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:462:  Tab found; better to use spaces  [whitespace/tab] [1]
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:464:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 50 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Therrell]

Created attachment 66507 [details]
2nd submission of patch w/ style corrections

[Oliver Hunt]

Comment on attachment 66507 [details]
2nd submission of patch w/ style corrections

Can you perf test this -- in general we don't compile in any logging in by default as JSC has a tendency to be heavily sensitive to branches and register load (using a global consumes a register as gcc's PIC code is not very clever)

[Oliver Hunt]

whoops didn't mean to r- this, just meant to get you to test perf numbers

[Alexey Proskuryakov]

Comment on attachment 66507 [details]
2nd submission of patch w/ style corrections

+        Reviewed by NOBODY (OOPS!).

There should be a bug reference and title in ChangeLog.

> Added Runtime checks do not impact performance on SunSpider benchmark.

SunSpider is what we use for most performance measurements, but adding mutexes and spinlocks isn't likely to affect it. Locking a mutex is very fast if no one else is holding it, and only parallel execution that's going to be affected by lock contention.

        static void enableStats() { s_statsEnabled = true; }

I don't think that this function needs to be inline. "Statistics" should be spelled out, and the name (or at least a comment) should explain what kind of statistics is enabled. We already track lots of memory-related statistics, and this call doesn't affect most.

It's not necessarily clear that statistics are only reported for (de)allocations that happen after enabling. It's particularly unclear and even wrong since totalBytesCommitted() can be negative, but returns a size_t.

+    SpinLockHolder lockHolder(&spinlock);
+    s_totalBytesCommitted += byteCount;

Ideally, this would use a variant of atomicIncrement rather than locking, but the version we have in Atomics.h can only add 1.

We don't use SpinLock outside of allocator code. It's part of FastMalloc.

If a Mutex doesn't give adequate performance, we should probably consider moving SpinLock to a general WTF header.

+        static size_t totalBytesCommitted() { return s_totalBytesCommitted; }

Looking at a shared value also requires locking in general case. A size_t variable may be unaligned or longer than processor word, in which cases reading is not an atomic operation.

+    void addToStats(size_t byteCount)
+    {
+        s_totalBytesCommitted += byteCount;
+    }

Why is it OK to not do locking for FixedVMPoolAllocator? If another lock is expected to be held, I think that we should assert so.

> Index: WebKit/mac/ChangeLog

I don't know what exactly the requirements are, but I expected to see at least Windows and WebKit2 here, too.

++ (void)enableJavaScriptStatistics
+{
+    RegisterFile::enableStats();
+    ExecutableAllocator::enableStats();
+}

It's very unclear what statistics this method enables. Who is going to call it, and when?

This is only a low level review, someone more familiar with JSC and allocator code should tell if these are good things to report in general.

[John Therrell]

Created attachment 66553 [details]
Submission of patch with suggested changes added

-Added link to bug in changelogs
-Renamed functions following suggestion in comments. Function naming follows FastMalloc statistics naming convention.
-Add comment to explain requirement to call enableStatistics before any allocations are made in order to reflect accurate, non-negative byte counts.
-Add assertion in FixedVMPoolAllocator that locks are held.
-Removed inline functions following suggestion in comments.

[John Therrell]

Created attachment 66825 [details]
Revised patch w/ use of Mutex in RegisterFile and runtime enabling/disabling removed

[John Therrell]

(In reply to comment #8)
> Created an attachment (id=66825) [details]
> Revised patch w/ use of Mutex in RegisterFile and runtime enabling/disabling removed

Ran performance testing to ensure patch doesn't impact JSC Performance. Here are the results:

Before patch (SunSpider run 30 times)
--------------------------------------------
Total:                 211.1ms +/- 0.4%
--------------------------------------------

After patch (SunSpider run 30 times)
--------------------------------------------
Total:                 207.5ms +/- 0.4%
--------------------------------------------

[Alexey Proskuryakov]

Comment on attachment 66825 [details]
Revised patch w/ use of Mutex in RegisterFile and runtime enabling/disabling removed

> +        static size_t s_totalBytesCommitted;
> +        Mutex mutex;

"mutex" is not a good name for the new member variable. It doesn't convey any meaning, and it should start with "m_". I'd suggest "m_statisticsMutex".

But most importantly, the variable it's protecting is static, so there should be only one instance of the mutex. Locking one mutex has no effect on other instances, so several RegisterFile can currently write into s_totalBytesCommitted simultaneously, which is a problem.

> +        ASSERT(IsHeld(spinlock));

IsHeld() is a member function, so this should be spinlock.IsHeld().

Please make sure that this patch doesn't break debug builds. Please also run tests in debug builds, so that assertions would have a chance to fire.

> +    void addToStatistics(size_t byteCount)
> +    {
> +        s_totalBytesCommitted += byteCount;
> +    }

The ASSERT() should be where we actually need it to hold - so it's better to put it here, and not at call site.

+size_t RegisterFile::registerFileStatistics()
+{
+    return s_totalBytesCommitted;
+}
...
> +size_t FixedVMPoolAllocator::fixedVMPoolAllocatorStatistics()
> +{
> +    return s_totalBytesCommitted;
> +}

These should also use locking. Otherwise, we may be reading while another thread writes, and get a partially overwritten value.

> +size_t ExecutableAllocator::executableAllocatorStatistics()
> +{
> +    return FixedVMPoolAllocator::fixedVMPoolAllocatorStatistics();
> +}

I don't see why two functions are needed. Isn't just the static version sufficient?

> +    size_t jscStackBytes = RegisterFile::registerFileStatistics();
> +    size_t jscJITBytes = ExecutableAllocator::executableAllocatorStatistics();

It seems like these function names can be further improved. "Statistics" is much less informative than what you have e.g. for these variable names.

[John Therrell]

Created attachment 66979 [details]
Revised patch w/ changes in Mutex use and function renaming

Built both debug and release builds without error. Performance testing via SunSpider confirms no performance regression caused by patch: Ran webkit-tests without issue. Made changes following review earlier this morning.

With Patch: (SunSpider run 30 times)
--------------------------------------------
Total:                 207.6ms +/- 0.3%
--------------------------------------------

[WebKit Review Bot]

Attachment 66979 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:444:  lock_holder is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Therrell]

Created attachment 66982 [details]
Same patch as above with style error resolved (Revised patch w/ changes in Mutex use and function renaming)

Same patch as above. I only renamed a SpinLockHolder from lock_holder to lockHolder.

[Alexey Proskuryakov]

Comment on attachment 66982 [details]
Same patch as above with style error resolved (Revised patch w/ changes in Mutex use and function renaming)

> +        void addToCommittedByteCount(size_t byteCount);

The argument name is not needed here, it doesn't add any information. I'm not sure if this is going to build with all compilers - you pass intptr_t to this function, so there is a suspicious signed to unsigned conversion. It's also confusing that this function takes a size_t, but is frequently called with negative values. I think that the argument can be a long.

> +static FixedVMPoolAllocator* allocator = 0; 

There is a stray space at the end of the line.

Looks good to me.

[Stephanie Lewis]

This was landed by John before he left for the summer


http://trac.webkit.org/projects/webkit/changeset/67130

and related commits:

http://trac.webkit.org/projects/webkit/changeset/67138
http://trac.webkit.org/projects/webkit/changeset/67265