Attempt to reduce performance penalty for logging and profiling

Attempt to reduce performance penalty for logging and profiling


Committed: http://code.google.com/p/v8/source/detail?r=2263

[Mikhail Naganov, 2009-06-16 20:01:13]

I like your change. How serious is the performance improvement it provides?

http://codereview.chromium.org/125141/diff/1003/11
File src/log.h (right):

http://codereview.chromium.org/125141/diff/1003/11#newcode243
Line 243: inline static bool IsEnabled() {
I think you can replace calls to Logger::IsEnabled with calls to
Logger::is_enabled, and get rid of this function.

http://codereview.chromium.org/125141/diff/1003/11#newcode261
Line 261: inline static bool is_enabled() { return is_enabled_; }
A member function defined within the class definition is 
taken to be an inline member function, so "inline" is redundant here.

http://codereview.chromium.org/125141/diff/1003/11#newcode344
Line 344: // VMState class implementation.  A simple stack of VM states held by
the
I suggest to put this implementation into a log-inl.h file, as C++ Style Guide
recommends:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=The_-i...

[antonm, 2009-06-23 15:35:35]

Misha, thanks a lot for both review and your help offline.

http://codereview.chromium.org/125141/diff/1003/11
File src/log.h (right):

http://codereview.chromium.org/125141/diff/1003/11#newcode243
Line 243: inline static bool IsEnabled() {
On 2009/06/16 20:01:13, Mikhail Naganov wrote:
> I think you can replace calls to Logger::IsEnabled with calls to
> Logger::is_enabled, and get rid of this function.

I renamed all the methods to is_logging

http://codereview.chromium.org/125141/diff/1003/11#newcode261
Line 261: inline static bool is_enabled() { return is_enabled_; }
On 2009/06/16 20:01:13, Mikhail Naganov wrote:
> A member function defined within the class definition is 
> taken to be an inline member function, so "inline" is redundant here.

Oh yes, thanks

http://codereview.chromium.org/125141/diff/1003/11#newcode344
Line 344: // VMState class implementation.  A simple stack of VM states held by
the
On 2009/06/16 20:01:13, Mikhail Naganov wrote:
> I suggest to put this implementation into a log-inl.h file, as C++ Style Guide
> recommends:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=The_-i...

Thanks a lot, done

[Mikhail Naganov, 2009-06-24 07:36:39]

Please also add 'log-inl.h' into Visual Studio project files
(tools/visual_studio) and into XCode project. I know, this is pain, but .gyp
file is currently only used in Chromium. For a standalone V8, project files are
still used.

http://codereview.chromium.org/125141/diff/3012/3020
File src/log.h (right):

http://codereview.chromium.org/125141/diff/3012/3020#newcode31
Line 31: #include "platform.h"
Is this include really needed? I see you've added a forward declaration of class
Semaphore there, but it already presents in this file.

[antonm, 2009-06-24 13:04:19]

Thanks a lot, Misha!

I hope I didn't screw proj files.

http://codereview.chromium.org/125141/diff/3012/3020
File src/log.h (right):

http://codereview.chromium.org/125141/diff/3012/3020#newcode31
Line 31: #include "platform.h"
On 2009/06/24 07:36:39, Mikhail Naganov wrote:
> Is this include really needed? I see you've added a forward declaration of
class
> Semaphore there, but it already presents in this file.

Alas, they both needed

[Mikhail Naganov, 2009-06-24 13:06:42]

LGTM

Great! I'm happy that presence of profiling code no more affects performance.