Revamp v8_optimized_debug options

Revamp v8_optimized_debug options

This patch changes the definition of v8_optimized_debug==1 to match the release-mode compiler optimization settings (generally, going from -O1 to -O3 on Linux, similar switches for Mac/Win). This produces a minor speed up on Linux, but significant speedups on Mac and Win. This may make it much harder to debug, though.

It also adds a v8_optimized_debug==2 that, in addition to the compiler optimizations, undef's DEBUG and defines DEBUG. This leaves V8_ENABLE_CHECKS alone (so that the assertions are still enabled), but otherwise basically matches a release mode build.

Builds with v8_optimized_debug==2 roughly match a Release mode build for speed; the V8_ENABLE_CHECKS checks appear to have minimal performance impact (maybe 5-10%, unlike what was previously thought). In addition, switching from the previous optimization settings makes a significant improvement on Mac and Win (50% or more), and switching from DEBUG to NDEBUG makes a significant improvement (another 50% or more). 

Note that using v8_optimized_debug==2 may also cause some v8 tests to fail. This is currently is believed to be acceptable.

R=machenbach@chromium.org
BUG=254188
Committed: https://code.google.com/p/v8/source/detail?r=15937

[jochen (gone - plz use gerrit), 2013-07-22 11:08:14]

+jkummerow

Jakob, wdyt?

[Michael Achenbach, 2013-07-22 11:23:19]

https://codereview.chromium.org/19384011/diff/1/build/toolchain.gypi
File build/toolchain.gypi (right):

https://codereview.chromium.org/19384011/diff/1/build/toolchain.gypi#newcode66
build/toolchain.gypi:66: # 3 - match release mode optimizations
The issue description introduces mode=2, why is it 3 here?

https://codereview.chromium.org/19384011/diff/1/build/toolchain.gypi#newcode508
build/toolchain.gypi:508: 'defines!': ['_DEBUG',],
Is the "_" intentional?

[Jakob Kummerow, 2013-07-22 12:41:50]

Providing a way to enable further compiler optimizations is probably fine, as
long as it's opt-in. By default, a Debug build should be debuggable, and with
aggressive compiler optimizations it's not.
Last time machenbach@ tried -O3 in Debug mode, that failed to compile, so please
make sure there are no issues on either of our supported toolchains (GCC, Clang,
MSVS, Xcode).

I don't think un-#defining DEBUG is a good idea. Debug-mode checks are there for
a reason, and that's not just to guard against v8-internal bugs. They're also
supposed to catch bugs in embedding code (such as feeding bad values to V8 API
calls), and for this purpose experience says we should have more checks rather
than fewer. V8_ENABLE_CHECKS is specifically for checks of this kind, it should
not be turned off for any build that tests changes against the V8 API.
I suspect that when such a switch exists, and the rumor spreads that specifying
it is a good way to speed up builds, people will do it, bugs will get
overlooked, and everyone's lives will be harder. 

Isn't it a sane model to have Release bot runs for quick feedback, and Debug
runs for full coverage?

(As an aside, ENABLE_DISASSEMBLER, OBJECT_PRINT and VERIFY_HEAP don't affect
performance unless certain command-line switches are provided at run time, so
there's no point in turning them off.)

[Dirk Pranke, 2013-07-22 20:55:36]

Jochen sent this out for review before I was quite ready to have it
reviewed :). I would have adjusted the description at least to make it
clearer what I was trying to do (and fixed the obvious inconsistencies).

From what I can tell, there's 3-4 different potential use cases for devs:

1) Build with all of the debug checks enabled, should be able to debug
anything, don't care about optimizations / compile speeds.

2) Same as one, but we want things to run as fast as possible without
losing any checking or debugging (optimize as much as possible).

3) Care about speed enough that it overrides debugging (don't need to
debug). Optimize fully.

4) Still too slow - disable the checks as well.

There's also the "test what we release" configuration, which I'll ignore
for the rest of this thread ...

Historically, we've had (1) and (4) w/ Chromium, and the 'dcheck_always_on'
configuration is basically (3). Chromium has not cared about debug
performance, but WebKit did (and does); in addition, v8-centric tests are
dramatically slower in Debug mode than most Chromium debug tests. As a
result a number of ex-WebKit Blink devs find debug so slow that they only
run Release. In addition, the WebKit assert macros don't have the
equivalent of a 'dcheck_always_on', and so we don't have (3). This means
that Blink devs are already missing the checks they'd like to get.

So, we're trying to fix this. Ideally, we'd get the (2) option fast enough
so that we don't really need (3) or (4). There's a bunch of profiling we
can still do, but it's clear that the simple gyp-flag-tweaking we've done
thus far isn't good enough (4x - 5x slower is too slow).

This patch has two aspects. The first is an attempt to make the (2) option
better. It changes the debug flags triggered by v8_optimized_debug to have
more of an effect on Mac and Win; the current flags (-O1) show marginal
improvement over (-O0). It's not yet clear to me how much debuggability is
lost by this, but since these changes are confined to the v8 code itself,
at least it doesn't effect debugging blink code, just v8 code. From the
point of view of blink (non-v8) devs -- and there's quite a lot of them --
this is a perfectly reasonable tradeoff since they neither need nor care
about debugging v8 code most of the time (v8 is almost a system lib).

The second part of the patch -- the v8_enable_checks_in_debug part -- is an
attempt to get something closer to the 'dcheck_always_on' configuration
*for Blink asserts*. This is equivalent to "build debug blink and link
against a release v8", but of course there's no good way to just get that
in GYP. Perhaps I should rename the flag to something like
'build_release_v8_in_debug' instead.

Ideally, we'd also keep the v8 asserts, but that appears to still cause at
least a 2x slowdown. Subsequent work should still figure out if we can
trade off some of these asserts against other changes in how we compile
Blink, disable some of the Blink checks, etc.

Note that I don't want *either* of these settings on by default for v8
devs. I think it is reasonable to have something like (2) on by default for
Chromium devs. Also, the debug bots are too slow to be run by default on
try jobs, and so this means we don't get *any* assert coverage on try jobs
currently (v8 or blink). We want to get at least some coverage, either by a
fast debug or a 'dcheck_always_on' release mode, so things would get
strictly better on tryjob coverage than they are now.

On Mon, Jul 22, 2013 at 4:23 AM, <machenbach@chromium.org> wrote:

>
>
https://codereview.chromium.**org/19384011/diff/1/build/**toolchain.gypi<http...
> File build/toolchain.gypi (right):
>
> https://codereview.chromium.**org/19384011/diff/1/build/**
>
toolchain.gypi#newcode66<https://codereview.chromium.org/19384011/diff/1/build/toolchain.gypi#newcode66>
> build/toolchain.gypi:66: # 3 - match release mode optimizations
> The issue description introduces mode=2, why is it 3 here?
>

Turns out that the equivalent settings are 3 on Mac/Linux and 2 on Windows,
so I started out one way, and changed it to the other because having the
values be 0, 1, 3 either made sense or didn't depending on your platform of
choice :). I failed to update the description before Jochen forwarded this
to you, though.


> https://codereview.chromium.**org/19384011/diff/1/build/**
>
toolchain.gypi#newcode508<https://codereview.chromium.org/19384011/diff/1/build/toolchain.gypi#newcode508>
> build/toolchain.gypi:508: 'defines!': ['_DEBUG',],
> Is the "_" intentional?
>

Yes. Leaving that symbol defined causes either slowdowns or compile
failures or both (I don't recall which offhand, I will provide more
specifics to come).

On Mon, Jul 22, 2013 at 5:41 AM, <jkummerow@chromium.org> wrote:

> Providing a way to enable further compiler optimizations is probably fine,
> as
> long as it's opt-in. By default, a Debug build should be debuggable, and
> with
> aggressive compiler optimizations it's not.
>

See my comments above; I agree that a Debug build should be debuggable for
the components you care about debugging. I don't necessarily think *all*
code has to be debuggable. I think that's nice to have, but not necessarily
if it exacts too much of a performance hit.


> Last time machenbach@ tried -O3 in Debug mode, that failed to compile, so
> please
> make sure there are no issues on either of our supported toolchains (GCC,
> Clang,
> MSVS, Xcode).
>

Of course.


> I don't think un-#defining DEBUG is a good idea. Debug-mode checks are
> there for
> a reason, and that's not just to guard against v8-internal bugs. They're
> also
> supposed to catch bugs in embedding code (such as feeding bad values to V8
> API
> calls), and for this purpose experience says we should have more checks
> rather
> than fewer. V8_ENABLE_CHECKS is specifically for checks of this kind, it
> should
> not be turned off for any build that tests changes against the V8 API.
> I suspect that when such a switch exists, and the rumor spreads that
> specifying
> it is a good way to speed up builds, people will do it, bugs will get
> overlooked, and everyone's lives will be harder.
>
> Isn't it a sane model to have Release bot runs for quick feedback, and
> Debug
> runs for full coverage?
>

See my comments above. I agree that having more checks is good, but if it
makes things so slow that we don't (or can't afford to) run the "full
coverage" configuration, that doesn't help.


> (As an aside, ENABLE_DISASSEMBLER, OBJECT_PRINT and VERIFY_HEAP don't
> affect
>
performance unless certain command-line switches are provided at run time,
> so
> there's no point in turning them off.)
>

I agree that these don't appear to cause a perf hit. Unfortunately, if I
turn off the flags that do matter and leave these on, we get compile
failures. Those are probably separate issues that should be cleaned up
(e.g., protecting code by both #ifdef V8_ENABLE_CHECKS and VERIFY_HEAP, or
undef'ing VERIFY_HEAP if V8_ENABLE_CHECKS isn't defined, etc.). I can file
bugs for this if there's interest.

Does this all make more sense?

I will update the bug with many of these comments and more specific
benchmark numbers when I'm actually ready for this to be reviewed.

-- Dirk

[Dirk Pranke, 2013-07-23 02:02:00]

More notes, from only basic testing on Linux. I need to confirm these results on
Mac and Win and on the broader test suites.

https://codereview.chromium.org/19384011/diff/11001/build/toolchain.gypi
File build/toolchain.gypi (right):

https://codereview.chromium.org/19384011/diff/11001/build/toolchain.gypi#newc...
build/toolchain.gypi:447: 'VERIFY_HEAP',
Okay, from *initial* testing on linux, it looks like the main perf impact from
#defines between debug and release comes from defining DEBUG, not
V8_ENABLE_CHECKS or any of the other flags. (The time taken to run fast/html
drops from 30 seconds to 18).

[ I think that V8_ENABLE_CHECKS is still defined even if you #undef DEBUG, but I
need to double-check this (or have someone confirm). ]

At any rate, that means that really the thing that matters is changing from
DEBUG to NDEBUG, and we can leave V8_ENABLE_CHECKS always enabled in Debug mode.
Switching from -O1 to -O3 seems to maybe give an additional 5% boost. It's
unclear to me what the impact of this is on debuggability.

(again, this is *just* linux

https://codereview.chromium.org/19384011/diff/11001/build/toolchain.gypi#newc...
build/toolchain.gypi:474: 'BasicRuntimeChecks': '0',
This was a flag missed on Windows that would cause compile errors there.

https://codereview.chromium.org/19384011/diff/11001/build/toolchain.gypi#newc...
build/toolchain.gypi:513: ],
See comments above. This also means that 'v8_enable_checks_in_debug' is the
wrong name for this variable :), but I haven't fixed this yet.

[Dirk Pranke, 2013-07-24 00:51:50]

Okay, this is a dramatically revised patch. From much more extensive testing, it
looks like the slowdowns in debug mode result from two things:

1) Compiler optimizations: On Linux, switching from -O0 to -O1 gets you most of
the way there, but on Mac and Linux, you don't really get big improvements until
you match the release settings fully (-O1 is actually slower than -O0 on mac,
apparently).

2) Cflags: contrary to what I originally though, the problem is *not* #ifdef
V8_ENABLE_CHECKS. The problem is #ifdef DEBUG; switching to undef DEBUG / define
NDEBUG makes the difference. We can leave V8_ENABLE_CHECKS alone.

Based on this, if we make these two changes, then running the webkit_tests is
roughly 2.25x slower for chromium in debug mode than in release mode (and debug
v8 is maybe 5-10% slower than a release v8 dll, if that). 

Of course, this means that debugging v8 will be substantially harder, but it's
not clear if enough people want to do this to justify a separate flag setting.

And, of course, we'll leave this off by default in v8 (but enabled by default in
chromium/blink).

I've tested this on Linux/Mac/Win, both release and debug (and with both gcc and
clang on Linux) and everything seemed to still work (running webkit_tests, at
least; I didn't run any of the v8 tests).

Please take a look and/or land if this looks and sounds good to you?

[Michael Achenbach, 2013-07-24 08:12:23]

> I've tested this on Linux/Mac/Win, both release and debug (and with both gcc
and
> clang on Linux) and everything seemed to still work (running webkit_tests, at
> least; I didn't run any of the v8 tests).

Some V8 tests fail when compiling standalone (e.g.: optdebug=on make
x64.debug.check). Some more things might need to be switched off when using this
flag (optdebug=on sets v8_optimized_debug to 1).

Furthermore: We are already using v8_optimized_debug=1 on the V8 waterfall for
most of the standalone debug builds now. There we require running the internal
checks (DEBUG enabled). Therefore, it was better to have a separate option (e.g.
v8_optimized_debug=2) for even faster debug builds.

[jochen (gone - plz use gerrit), 2013-07-24 11:27:21]

On 2013/07/24 08:12:23, machenbach wrote:
> > I've tested this on Linux/Mac/Win, both release and debug (and with both gcc
> and
> > clang on Linux) and everything seemed to still work (running webkit_tests,
at
> > least; I didn't run any of the v8 tests).
> 
> Some V8 tests fail when compiling standalone (e.g.: optdebug=on make
> x64.debug.check). Some more things might need to be switched off when using
this
> flag (optdebug=on sets v8_optimized_debug to 1).

What exactly is failing? Are the tests failing because of bugs in V8?

> 
> Furthermore: We are already using v8_optimized_debug=1 on the V8 waterfall for
> most of the standalone debug builds now. There we require running the internal
> checks (DEBUG enabled). Therefore, it was better to have a separate option
(e.g.
> v8_optimized_debug=2) for even faster debug builds.

[Michael Achenbach, 2013-07-24 13:15:24]

> What exactly is failing? Are the tests failing because of bugs in V8?

No. I compared
optdebug=on make x64.debug.check
vs
optdebug=off make x64.debug.check

One problem seems to be the warning "unknown flag --enable-slow-asserts", which
is printed to stdout and which lets all "message" tests fail, because they
compare output 1:1. So this needs to be switched off when DEBUG isn't enabled.
It is set by the tools/run-tests.py command for debug builds (independent of any
optimizations).

Then there are errors of this type:
./test/mjsunit/fuzz-natives-part2.js:225: TypeError: Object [object builtins]
has no method 'ListNatives'
  var allNatives = %ListNatives();

It seems like the built-in natives are not built.

There seem to be some assumptions about what is to be done with a "DEBUG" build.
Maybe instead of changing the configuration of the debug build too much, we
should add a third build type with a new name?

[Jakob Kummerow, 2013-07-24 14:53:24]

Both examples can be summed up as: V8's test system has different
expectations/behavior for Release and Debug builds. Some differences are due to
performance (longer timeouts), some are due to debugging features that aren't
even compiled in Release mode (by means of #ifdef DEBUG guards).

I think this is another argument for having three modes:
(1) v8_optimized_debug=0 for regular debug builds, 
(2) v8_optimized_debug=1 for optimized (-O{1,2,3}) builds with identical checks
and functionality as the "0" setting, 
(3) and finally v8_optimized_debug=2 (or =3 if you prefer) for a "this really
behaves like a Release build but is called Debug due to GYP's lack of
flexibility" mode. 

In that scenario, I don't think we urgently need to teach the V8 testing
framework to handle the third mode, nor have convenience targets in our Makefile
for it. Its use case would be embedders (and their waterfalls) who don't care
what V8 is doing internally. This means that V8_ENABLE_CHECKS should still be
defined in this mode, and we might consider moving some more checks behind that
#define that are of critical importance to embedders (off the top of my head I
don't know if there are any such checks).

[Dirk Pranke, 2013-07-24 17:38:29]

I had thought that chromium was the original motivation for v8_optimized_debug
in the first place, so it wasn't clear to me if you had a desire for an
optimized debug as well (though I know Michael's patch to add it overlapped with
mine, so this is perhaps not surprising). It's fine w/ me to support both modes;
I don't expect we'll use the "=1" setting in Chromium much if at all but maybe
someone working on debugging chromium and v8 together will want or need it and
not need the full "=0" debuggability.

I will add a "=2" setting. It doesn't quite "really behave like a release build"
since V8_ENABLE_CHECKS is still defined (and that isn't defined in Release
builds), but I don't think that's all that important.

We can always delete the =1 setting later if it turns out to be unneeded or
unused.

[Dirk Pranke, 2013-07-24 19:17:30]

Okay, =2 setting added back in. Please take another look?

[Michael Achenbach, 2013-07-25 09:19:34]

Still can't compile v8_optimized_debug=1 for the same reason as in
https://codereview.chromium.org/18431003/

Can +1 developer check this to make sure there's not just something screwed up
in my build environment?

Apply this patch and try:
optdebug=on make x64.debug.check

[Jakob Kummerow, 2013-07-25 16:46:09]

On 2013/07/25 09:19:34, machenbach wrote:
> Still can't compile v8_optimized_debug=1 for the same reason as in
> https://codereview.chromium.org/18431003/
> 
> Can +1 developer check this to make sure there's not just something screwed up
> in my build environment?
> 
> Apply this patch and try:
> optdebug=on make x64.debug.check

Yes, fails here too. First error is:

In file included from ../src/conversions.cc:32:0:
../src/conversions-inl.h: In function ‘double
v8::internal::InternalStringToDouble(v8::internal::UnicodeCache*, Iterator,
EndMark, int, double) [with Iterator = const short unsigned int*, EndMark =
const short unsigned int*]’:
../src/conversions-inl.h:696:3: error: array subscript is above array bounds
[-Werror=array-bounds]
../src/conversions-inl.h: In function ‘double
v8::internal::InternalStringToDouble(v8::internal::UnicodeCache*, Iterator,
EndMark, int, double) [with Iterator = const char*, EndMark = const char*]’:
../src/conversions-inl.h:696:3: error: array subscript is above array bounds
[-Werror=array-bounds]
cc1plus: all warnings being treated as errors

GCC 4.6.3 as shipping with Ubuntu 12.04.

[Dirk Pranke, 2013-07-25 19:54:42]

Ah, I see. I'm not seeing this in the chromium builds because we turn off -Wall
for third party code like v8.

From initial appearances the code looks correct and this looks like a compiler
bug. I need to dig in some more to confirm and figure out what to do about it;
I'm not sure why this is only showing up w/ these flags yet.

[Dirk Pranke, 2013-07-25 22:26:34]

It looks like it's some combination of -Wall (which enables the array bounds
check), -O2/-O3, and #ifdef DEBUG, which is enabling the assert code and
possibly triggering a compiler bug. I'll file a separate bug to track that and
post a patch that works around it for now and you can decide what course of
action you'd prefer.

[Dirk Pranke, 2013-07-25 22:33:03]

On 2013/07/25 22:26:34, Dirk Pranke wrote:
> It looks like it's some combination of -Wall (which enables the array bounds
> check), -O2/-O3, and #ifdef DEBUG, which is enabling the assert code and
> possibly triggering a compiler bug. I'll file a separate bug to track that and
> post a patch that works around it for now and you can decide what course of
> action you'd prefer.

File https://code.google.com/p/v8/issues/detail?id=2807 .

[Dirk Pranke, 2013-07-25 23:13:09]

Okay, I've posted a patch that changes the linux setting to -O1 .

Depending on how you want to address the compiler issue, we can either change
the code as suggested in https://code.google.com/p/v8/issues/detail?id=2807#c1,
or leave things as-is (and remove the TODO), or look into other alternatives.

As noted, there's a minor perf difference between -O1 and -O3 w/ gcc (maybe
10%), so just sticking w/ -O1 should be acceptable for the indefinite future.

[Michael Achenbach, 2013-07-26 12:05:24]

I can live with the v8_optimized_debug=1 setting for now. But I wonder where did
the -O3 for linux in v8_optimized_debug=2 go? Is it completely removed now? I
thought the idea was:

Level 0:
-O0
DEBUG

Level 1:
-O1
DEBUG

Level 2:
-O3
NDEBUG

Level 0 is for debugging, level 1 is for testing debug on the V8 waterfall
(including our chromium builds) and level 2 is for embedders.

Can't say much about Windows and Mac... lets let the buildbots decide on that.

[Dirk Pranke, 2013-07-26 16:50:25]

I don't feel strongly about -O1 vs. -O3 in level 2; the performance difference
is minimal. I left it as -O1 just to keep things simpler in the gyp file. I'll
post a patch that is -O3. 

It would be great if you or jkummerow could lgtm one of these variants and land
it if you're basically happy with things; a lot of blink and chromium devs would
appreciate getting this in and we can always tweak things later if we need to.

[Michael Achenbach, 2013-07-26 20:30:02]

LGTM

[Dirk Pranke, 2013-07-26 21:44:14]

On 2013/07/26 20:30:02, machenbach wrote:
> LGTM

Thanks! I've posted the -O3 patch, so if you could land this that would be
great.

[Dmitry Lomov (no reviews), 2013-07-29 13:59:48]

On 2013/07/26 21:44:14, Dirk Pranke wrote:
> On 2013/07/26 20:30:02, machenbach wrote:
> > LGTM
> 
> Thanks! I've posted the -O3 patch, so if you could land this that would be
> great.

Rubberstamp LGTM

[Dmitry Lomov (no reviews), 2013-07-29 14:00:16]

Committed patchset #7 manually as r15937 (presubmit successful).

[Michael Achenbach, 2013-07-31 16:48:25]

On 2013/07/29 14:00:16, Dmitry Lomov (chromium) wrote:
> Committed patchset #7 manually as r15937 (presubmit successful).

It is very likely that this CL causes a crash in the mozilla tests under windows
debug:

http://chromegw.corp.google.com/i/client.v8/builders/V8%20Win32%20-%20debug%2...

It got landed in rev 15937 and in the revisions after, some odd problems started
that can't be attributed to anything else. Since I didn't clobber this time it
can be that only portions were compiled with the new options and as more files
were compiled in subsequent builds, the problem changed and was also a bit
flaky. By now, we clobbered but have this stable crash left.

About this CL:
(1) There seems to be an inconsistency in the msvs_settings. The
component=="shared_library" condition is once outside and once inside the
v8_optimized_debug==0 condition. I am not sure which combination (3 or 1) or (2
or 0) is matching here. I also don't know what it means.

(2) We might need to get back where we were before that CL for the
v8_optimized_debug==1 case in windows, so that we have a condition for each of
the three debug cases. I don't care what case 2 contains, as long as it works
for embedders. But case 1 needs to work for v8 standalone on all platforms.

[Jakob Kummerow, 2013-07-31 16:55:35]

On 2013/07/31 16:48:25, machenbach wrote:
>
http://chromegw.corp.google.com/i/client.v8/builders/V8%2520Win32%2520-%2520d...

Proper URL:
http://build.chromium.org/p/client.v8/builders/V8 Win32 - debug - mozilla -
1?numbuilds=200
or
http://goo.gl/GHAVdy

[Dirk Pranke, 2013-07-31 16:57:30]

Well, you should certainly feel free to revert the change and see if that solves
your problems. I will take another look at the code shortly to make sure I got
the msvs_settings right.

I will note that we haven't flipped to 2 yet in chromium, so we're still running
with 1. I haven't heard any reports of problems yet from our bots (of course,
that doesn't mean that they don't exist, or maybe it's just the mozilla tests
hitting something).

[Michael Achenbach, 2013-07-31 17:01:27]

I don't want to revert the whole thing (I love the speed up on mac). But could
you clarify point (1) I talked about? Which RuntimeLibrary setting should be
used where?

Regarding (2), I will make a new CL tomorrow.

[Dirk Pranke, 2013-07-31 17:13:43]

On 2013/07/31 17:01:27, machenbach wrote:
> I don't want to revert the whole thing (I love the speed up on mac). But could
> you clarify point (1) I talked about? Which RuntimeLibrary setting should be
> used where?
> 
> Regarding (2), I will make a new CL tomorrow.

Looks like danno did revert this in
https://code.google.com/p/v8/source/detail?r=15975 ?

[Jakob Kummerow, 2013-07-31 17:15:31]

On 2013/07/31 17:13:43, Dirk Pranke wrote:
> Looks like danno did revert this in
> https://code.google.com/p/v8/source/detail?r=15975 ?

Nope, that was a trunk commit. This patch is still on the bleeding_edge branch.

[Dirk Pranke, 2013-07-31 17:17:12]

On 2013/07/31 17:15:31, Jakob wrote:
> Nope, that was a trunk commit. This patch is still on the bleeding_edge
branch.

Ah, getting reverted on trunk and having that revert rolled into chromium counts
as a revert to me :) .

[Dirk Pranke, 2013-07-31 17:27:33]

On 2013/07/31 16:48:25, machenbach wrote:
> About this CL:
> (1) There seems to be an inconsistency in the msvs_settings. The
> component=="shared_library" condition is once outside and once inside the
> v8_optimized_debug==0 condition. I am not sure which combination (3 or 1) or
(2
> or 0) is matching here. I also don't know what it means.
> 

The flag is documented here:
http://msdn.microsoft.com/en-us/library/2kzt1wy3.aspx 

It controls which version of the C runtime library we compile and link against. 

/MT (0) links against libcmt (the static release lib, multi-thread safe), which
we should be using in a release (component==) static build

/MD (2) is the dll release version (release component == shared)

/MTd (1) is the debug static, and /MDd the debug shared flags. 

I *think* the later conditions override the earlier conditions, so this logic
should (?) be correct. It is admittedly confusing and should probably be
reworked, as should the whole use of configuration-specific settings here. 

It's also possible that we're seeing issues with parts of the code compiled
against the release libc (w/o _DEBUG) and other parts having the debug libc;
I've seen at least one issue of this type on Android Clang (you were cc'ed on
the bug), so it's possible that we'll have to take a different approach on some
of the platforms.

I need to log in to my windows machine to re-run gyp and confirm what is
actually being generated, so I'll do that in a separate post.

[Dirk Pranke, 2013-07-31 17:37:35]

On 2013/07/31 17:27:33, Dirk Pranke wrote:
> I need to log in to my windows machine to re-run gyp and confirm what is
> actually being generated, so I'll do that in a separate post.

Yup, looks like the code is being generated as I would expect. Note that Visual
Studio will also generate warnings (possibly fatal warnings/errors, I don't
recall for sure at the moment) if you are linking code that was compiled with
two different settings for this flag, and presumably we're not seeing those.

[Dirk Pranke, 2013-07-31 17:51:09]

Hm, in chromium, with v8_optimized_debug==2, I'm seeing both /MD (release dll)
and _DEBUG set as compiler flags, and that seems bad. Perhaps some other gyp
file is setting _DEBUG somewhere (which I think I had to undef in an earlier
version of this patch ...). 

And, with v8_optimized_debug==1 , I'm still seeing /MD instead of /MDd. I think
that part of the patch might be bad.

So, it probably makes sense to revert this altogether (and verify that the
mozilla tests no longer fail) and for me to post a new version that is clearer
that I can do some more sanity-checking for flag consistency on. I also need to
double-check these aspects with a windows guru to be sure I have it right.

[Dirk Pranke, 2013-07-31 18:53:02]

Thinking about this a bit more, I should probably split this into a couple of
patches that make smaller changes that are less disruptive (e.g., one that fixes
mac and win perf with v8_opt_debug==1, and then one or more for ==2). I'll work
up some patches along those lines. Let me know if you end up reverting this in
the meantime, so I can rebase them, but I'll for now I'll assume you won't
revert anything before I can post new patches.