Fix performance regressions due to lazy initialization.

Fix performance regressions due to lazy initialization.

This CL:
- Adds a new trait parameter to LazyInstance to let it initialize the instance
  without paying the cost of atomic operations (which are expensive on Mac).
  This only works for users who don't care about thread-safety and this is now
the default initialization trait used by LazyInstance in v8.

- Reverts the changes that were made in r11010 in isolate.{cc,h}. That lets
  Isolate's accessors be as cheap as they were before (but adds one static initializer).

- Adds OS::PostSetup() used to initialize the math functions which depend on CPU features.
That lets the math functions get rid of CallOnce().

BUG=118686
Committed: https://code.google.com/p/v8/source/detail?r=11198

[danno, 2012-03-29 07:45:16]

It's getting there. Did you run Florian's micro-benchmark with this?

https://chromiumcodereview.appspot.com/9873023/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/isolate.cc#newcode319
src/isolate.cc:319: Mutex* Isolate::process_wide_mutex_ = OS::CreateMutex();
Shouldn't you initialize this from GlobalSetup? Seems like it causes code
execution for a static initializer in its current form.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h
File src/lazy-instance.h (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:218: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
how about changing the default to SingleThreadInitOnceTrait for V8? This is by
far the most frequent usage pattern, I suspect. It might simplify code
elsewhere.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:228: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
Here too

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:239: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
Here too

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc#n...
src/platform-posix.cc:137: if (!fast_##name##_function) {                       
 \
Since this is also could be called frequently, perhaps it makes sense to
initialize the function list from InitializeOncePerProcess?

[Philippe, 2012-03-29 09:00:48]

I didn't run Florian's benchmark as I planned initially since it evaluates
Isolate::Current() performance which is now exactly the same as it was before.
We can be 100% sure that if the regression comes from Isolate's accessors (which
I'm hoping), then it will be gone with this CL.

I'm waiting for your opinion about adding OS::PostSetup() before I send a new
patch set.

Thanks,
Philippe.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/isolate.cc#newcode319
src/isolate.cc:319: Mutex* Isolate::process_wide_mutex_ = OS::CreateMutex();
On 2012/03/29 07:45:17, danno wrote:
> Shouldn't you initialize this from GlobalSetup? Seems like it causes code
> execution for a static initializer in its current form.

Indeed. I naively copied what we had before my initial patch  without even
thinking about that.

I had to add a call to Isolate::GlobalSetup() in d8.cc.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h
File src/lazy-instance.h (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:218: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
On 2012/03/29 07:45:17, danno wrote:
> how about changing the default to SingleThreadInitOnceTrait for V8? This is by
> far the most frequent usage pattern, I suspect. It might simplify code
> elsewhere.

Right. I was just concerned about the potential risk of making LazyInstance
non-thread-safe. But as you said in v8, that is frequent. I added a warning in
the top-level comment.

Done.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:228: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
On 2012/03/29 07:45:17, danno wrote:
> Here too

Done.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/lazy-instance.h#new...
src/lazy-instance.h:239: template <class> class InitOnceTrait =
ThreadSafeInitOnceTrait,
On 2012/03/29 07:45:17, danno wrote:
> Here too

Done.

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc#n...
src/platform-posix.cc:137: if (!fast_##name##_function) {                       
 \
On 2012/03/29 07:45:17, danno wrote:
> Since this is also could be called frequently, perhaps it makes sense to
> initialize the function list from InitializeOncePerProcess?

I tried to do that initially by calling these initialization functions once in
OS::SetUp(). The problem is that the functions used to create these math
functions (during OS SetUp) depend on CpuFeatures initialized by CPU::SetUp().
CPU itself depends on some OS functions that require initialization (which is
why in InitializeOncePerProcess(), OS is initialized before CPU). What we can do
though, is adding a function like OS::PostSetUp() for example which would be
called after CPU initialization. This function could initialize the math
functions.

What do you think?

[danno, 2012-03-29 09:07:20]

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc#n...
src/platform-posix.cc:137: if (!fast_##name##_function) {                       
 \
Calling it from OS::PostSetUp seems fine. I would definitely prefer that to the
lazy initialized version.

On 2012/03/29 09:00:48, Philippe wrote:
> On 2012/03/29 07:45:17, danno wrote:
> > Since this is also could be called frequently, perhaps it makes sense to
> > initialize the function list from InitializeOncePerProcess?
> 
> I tried to do that initially by calling these initialization functions once in
> OS::SetUp(). The problem is that the functions used to create these math
> functions (during OS SetUp) depend on CpuFeatures initialized by CPU::SetUp().
> CPU itself depends on some OS functions that require initialization (which is
> why in InitializeOncePerProcess(), OS is initialized before CPU). What we can
do
> though, is adding a function like OS::PostSetUp() for example which would be
> called after CPU initialization. This function could initialize the math
> functions.
> 
> What do you think?

[Philippe, 2012-03-29 09:26:53]

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc
File src/platform-posix.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/1/src/platform-posix.cc#n...
src/platform-posix.cc:137: if (!fast_##name##_function) {                       
 \
On 2012/03/29 09:07:20, danno wrote:
> Calling it from OS::PostSetUp seems fine. I would definitely prefer that to
the
> lazy initialized version.
> 
> On 2012/03/29 09:00:48, Philippe wrote:
> > On 2012/03/29 07:45:17, danno wrote:
> > > Since this is also could be called frequently, perhaps it makes sense to
> > > initialize the function list from InitializeOncePerProcess?
> > 
> > I tried to do that initially by calling these initialization functions once
in
> > OS::SetUp(). The problem is that the functions used to create these math
> > functions (during OS SetUp) depend on CpuFeatures initialized by
CPU::SetUp().
> > CPU itself depends on some OS functions that require initialization (which
is
> > why in InitializeOncePerProcess(), OS is initialized before CPU). What we
can
> do
> > though, is adding a function like OS::PostSetUp() for example which would be
> > called after CPU initialization. This function could initialize the math
> > functions.
> > 
> > What do you think?
> 

Done.

[danno, 2012-03-29 12:32:42]

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/isolate.cc#newco...
src/isolate.cc:373: // becase a non-null thread data may be already set.
nit: because

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/platform-cygwin.cc
File src/platform-cygwin.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/platform-cygwin....
src/platform-cygwin.cc:76: void MathSetup();  // Defined in platform-posix.cc.
I really don't like having this external declaration here. It seems like
overkill to create a platform-posix.h just to contain it, but I think I might
prefer it to the multiple platform-specific declarations.

[Philippe, 2012-03-29 13:06:53]

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/isolate.cc
File src/isolate.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/isolate.cc#newco...
src/isolate.cc:373: // becase a non-null thread data may be already set.
On 2012/03/29 12:32:42, danno wrote:
> nit: because

Done.

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/platform-cygwin.cc
File src/platform-cygwin.cc (right):

https://chromiumcodereview.appspot.com/9873023/diff/7014/src/platform-cygwin....
src/platform-cygwin.cc:76: void MathSetup();  // Defined in platform-posix.cc.
On 2012/03/29 12:32:42, danno wrote:
> I really don't like having this external declaration here. It seems like
> overkill to create a platform-posix.h just to contain it, but I think I might
> prefer it to the multiple platform-specific declarations.

Done. Indeed. platform-posix.h might also serve later.

[danno, 2012-03-30 12:07:22]

lgtm

[danno, 2012-03-30 14:29:53]

lgtm