Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2689793003:
Enable string pooling in the no_optimize configuration, under Windows. (Closed)
DescriptionEnable string pooling in the no_optimize configuration, under Windows.
This matches the default behaviour, even in Debug builds, of GCC & Clang.
AllocationContextTracker::PseudoStackFrame actually relies on string
pooling to allow it to compare trace event category and name strings by
address rather than strcmp().
BUG=686208
Review-Url: https://codereview.chromium.org/2689793003
Cr-Commit-Position: refs/heads/master@{#449874}
Committed: https://chromium.googlesource.com/chromium/src/+/772ff4cd3df488b794fa66e8ed9d2951543a11ee
Patch Set 1 #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Enable string pooling in no-optimize configuration. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG= ========== to ========== Enable string pooling in the no_optimize configuration, under Windows. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG= ==========
wez@chromium.org changed reviewers: + brucedawson@chromium.org
We think relying on string pooling is evil and wrong, but let's land this to un-break allocator tracing on Windows debug builds, until we decide otherwise.
wez@chromium.org changed reviewers: + brettw@chromium.org - brucedawson@chromium.org
brucedawson->brettw :)
I'm nervous. Why *wouldn't* we want to do this?
On 2017/02/11 05:03:57, brettw (plz ping after 24h) wrote: > I'm nervous. Why *wouldn't* we want to do this? For the record, gcc has string pooling enabled by default (-fmerge-constants). My belief is that MSVC is just being uber conservative about disabling transforms in /O0.
On 2017/02/11 06:17:45, awong wrote: > On 2017/02/11 05:03:57, brettw (plz ping after 24h) wrote: > > I'm nervous. Why *wouldn't* we want to do this? > > For the record, gcc has string pooling enabled by default (-fmerge-constants). > My belief is that MSVC is just being uber conservative about disabling > transforms in /O0. So this would break break code that (a) expected string literals to be writable (b) expected string literals to be different locations in memory. Both of these behaviors would break in /02 as that implies /GF anyways, thus this should actually be safe to enable in no_optimization. In fact, it brings the object allocation behavior of no_optimization closer to production which is actually a bonus. I say this should go in regardless of whether or not we rely on stringpooling in the trace code.
You beat me to it, Albert! (a) is benign; any code relying on writable strings would already break in Release builds. (b) means broken code that compares strings by pointer, may work in Release but would break in Debug, but won't if we enable string pooling. I don't think that's sufficient to justify keeping Debug on un-pooled strings, though; better to use compiler warnings for that. IIUC string pooling is actually enabled by /O1 or above (at least there is a comment in BUILD.gn to that effect ;) On 10 February 2017 at 22:20, <ajwong@chromium.org> wrote: > On 2017/02/11 06:17:45, awong wrote: > > On 2017/02/11 05:03:57, brettw (plz ping after 24h) wrote: > > > I'm nervous. Why *wouldn't* we want to do this? > > > > For the record, gcc has string pooling enabled by default > (-fmerge-constants). > > My belief is that MSVC is just being uber conservative about disabling > > transforms in /O0. > > So this would break break code that > (a) expected string literals to be writable > (b) expected string literals to be different locations in memory. > > Both of these behaviors would break in /02 as that implies /GF anyways, > thus > this should actually be safe to enable in no_optimization. In fact, it > brings > the object allocation behavior of no_optimization closer to production > which is > actually a bonus. > > I say this should go in regardless of whether or not we rely on > stringpooling in > the trace code. > > https://codereview.chromium.org/2689793003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Enable string pooling in the no_optimize configuration, under Windows. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG= ========== to ========== Enable string pooling in the no_optimize configuration, under Windows. This matches the default behaviour, even in Debug builds, of GCC & Clang. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG=686208 ==========
The CQ bit was checked by wez@chromium.org
The CQ bit was unchecked by wez@chromium.org
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
okay, lgtm
The CQ bit was checked by wez@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1486841722993660, "parent_rev":
"e438788bc20f2ce97db42bd480a7f73dec6e2421", "commit_rev":
"772ff4cd3df488b794fa66e8ed9d2951543a11ee"}
Message was sent while issue was closed.
Description was changed from ========== Enable string pooling in the no_optimize configuration, under Windows. This matches the default behaviour, even in Debug builds, of GCC & Clang. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG=686208 ========== to ========== Enable string pooling in the no_optimize configuration, under Windows. This matches the default behaviour, even in Debug builds, of GCC & Clang. AllocationContextTracker::PseudoStackFrame actually relies on string pooling to allow it to compare trace event category and name strings by address rather than strcmp(). BUG=686208 Review-Url: https://codereview.chromium.org/2689793003 Cr-Commit-Position: refs/heads/master@{#449874} Committed: https://chromium.googlesource.com/chromium/src/+/772ff4cd3df488b794fa66e8ed9d... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/772ff4cd3df488b794fa66e8ed9d... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
