Make possible to check memory allocations inside chromium

Make possible to check memory allocations inside chromium
    
This patch implements UncheckedMalloc and UncheckedCalloc for Linux.
Previously it was only possible on Mac. The motivation is to let callers handle
OOM gracefully instead of aborting.

When tcmalloc is used this is implemented via a weak symbol that is overridden
by tcmalloc. This way we get around the problem that neither base cannot depend
on tcmalloc and vica versa. Unfortunately weak symbols are not supported on Windows.
To make that work on Windows one will have to do some build system craft that is similar
of what we do to replace the system malloc with tcmalloc.

This cl does not try to solve the more controversial problem of disallowing
the OOM handler for third party libraries under special circumstances.

BUG=73751
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258681

[kbalazs, 2013-10-31 21:37:52]

@avi: I'd like to you to share your opinion about the concept
@willchain: I'd like to ask you to review the patch

I'm almost sure it currently lacks a code path for Android, I need the trybots
to check this.

[Scott Hess - ex-Googler, 2013-10-31 21:54:38]

Drive-by comment.

https://codereview.chromium.org/55333002/diff/1/base/process/memory_linux.cc
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/1/base/process/memory_linux.cc#...
base/process/memory_linux.cc:206: memset(result, 0, size);
size -> alloc_size.

[Ken Russell (switch to Gerrit), 2013-10-31 22:12:48]

+dslomov, cevans

Thank you for implementing this!

I would very much like to hook this up to V8's typed array implementation so
that exceptions can be thrown upon allocation failure.

https://codereview.chromium.org/55333002/diff/1/base/process/memory.h
File base/process/memory.h (right):

https://codereview.chromium.org/55333002/diff/1/base/process/memory.h#newcode59
base/process/memory.h:59: // This can be useful for huge and/or unpredicteble
size memory allocatios.
typos: unpredictable, allocations

[Dai Mikurube (NOT FULLTIME), 2013-11-01 05:53:56]

Just a drive-by nit.

https://codereview.chromium.org/55333002/diff/90001/third_party/tcmalloc/READ...
File third_party/tcmalloc/README.chromium (right):

https://codereview.chromium.org/55333002/diff/90001/third_party/tcmalloc/READ...
third_party/tcmalloc/README.chromium:95: - Added tc_try_malloc, which is like
tc_malloc but it does not call the new handler
style nit: maybe over 80?

[Avi (use Gerrit), 2013-11-01 20:04:12]

This looks plausible but I don't know enough about the internals of the memory
code of Windows and Linux to say.

[Avi (use Gerrit), 2013-11-01 20:05:55]

On 2013/11/01 20:04:12, Avi wrote:
> This looks plausible but I don't know enough about the internals of the memory
> code of Windows and Linux to say.

And in case you're wondering, yes, I'm OK with this. I wish there were a
programmatic way to ensure that callers of UncheckedMalloc actually null-checked
the result (a la WARN_UNUSED_RESULT) but I don't see how to do that in a sane
manner.

[kbalazs, 2013-11-01 23:42:44]

> And in case you're wondering, yes, I'm OK with this. I wish there were a
> programmatic way to ensure that callers of UncheckedMalloc actually
null-checked
> the result (a la WARN_UNUSED_RESULT) but I don't see how to do that in a sane
> manner.

I was also thinking about WARN_UNUSED_RESULT but it does not give any guarantee
in this case.

[kbalazs, 2013-11-01 23:43:12]

Patches coming until it builds on every platform.

[Scott Hess - ex-Googler, 2013-11-01 23:58:16]

On 2013/11/01 23:42:44, kbalazs wrote:
> > And in case you're wondering, yes, I'm OK with this. I wish there were a
> > programmatic way to ensure that callers of UncheckedMalloc actually
> null-checked
> > the result (a la WARN_UNUSED_RESULT) but I don't see how to do that in a
sane
> > manner.
> 
> I was also thinking about WARN_UNUSED_RESULT but it does not give any
guarantee
> in this case.

There's no particular reason you have to continue using the NULL-return
interface, though.  You could have:

// If the buffer can be allocated, return true with the pointer in *pointer.
// Otherwise return false.
bool UncheckedMalloc(size_t size, void** pointer) WARN_UNUSED_RESULT;

Then if someone ignores the result, they are doing it willfully.

[Avi (use Gerrit), 2013-11-02 03:45:03]

On 2013/11/01 23:58:16, shess wrote:
> You could have:
> 
> // If the buffer can be allocated, return true with the pointer in *pointer.
> // Otherwise return false.
> bool UncheckedMalloc(size_t size, void** pointer) WARN_UNUSED_RESULT;
> 
> Then if someone ignores the result, they are doing it willfully.

Oooh! In any case I do like making the interface a bit weird so that someone has
to think to use it.

[willchan no longer on Chromium, 2013-11-02 21:11:44]

https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win.cc
File base/process/memory_win.cc (right):

https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win....
base/process/memory_win.cc:9: #include "base/allocator/unchecked_malloc.h"
Why?

https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
File third_party/tcmalloc/chromium/src/debugallocation.cc (right):

https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
third_party/tcmalloc/chromium/src/debugallocation.cc:1434: namespace base {
Sorry, this is not acceptable. You can't define a base function in tcmalloc.
This is a layering violation.

https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
File third_party/tcmalloc/chromium/src/tcmalloc.cc (right):

https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
third_party/tcmalloc/chromium/src/tcmalloc.cc:1723: namespace base {
Ditto here on the layering violation.

[willchan no longer on Chromium, 2013-11-03 02:04:44]

The right way to do this would be to expose a new tcmalloc function.
tc_malloc_unchecked() or something. And have base call that directly when we're
using TCMalloc.

[kbalazs, 2013-11-04 15:25:31]

> 
>
https://codereview.chromium.org/55333002/diff/180001/base/process/memory_win....
> base/process/memory_win.cc:9: #include "base/allocator/unchecked_malloc.h"
> Why?

Left behind by mistake.

> 
>
https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
> File third_party/tcmalloc/chromium/src/debugallocation.cc (right):
> 
>
https://codereview.chromium.org/55333002/diff/180001/third_party/tcmalloc/chr...
> third_party/tcmalloc/chromium/src/debugallocation.cc:1434: namespace base {
> Sorry, this is not acceptable. You can't define a base function in tcmalloc.
> This is a layering violation.

I see.

[kbalazs, 2013-11-04 15:26:03]

On 2013/11/03 02:04:44, willchan wrote:
> The right way to do this would be to expose a new tcmalloc function.
> tc_malloc_unchecked() or something. And have base call that directly when
we're
> using TCMalloc.

Ok, I will do that.

[kbalazs, 2013-11-04 15:46:15]

I'm confused by the win bot result. This is the error:
gyp: name 'win_use_allocator_shim' is not defined while evaluating condition
'win_use_allocator_shim==1' in E:\b\build\slave\win\build\src\build\all.gyp

I introduced this condition in common.gyp but this variable should get a default
value here:
https://code.google.com/p/chromium/codesearch#chromium/src/build/common.gypi&...

Any idea? Maybe the bot needs a clobber?

[kbalazs, 2013-11-04 20:37:09]

Changed the interface to make WARN_UNUSED_RESULT useful, fixed layering
violation.

[willchan no longer on Chromium, 2013-11-05 00:27:42]

I'm adding the gperftools maintainer to see what his thoughts are here.

[Ken Russell (switch to Gerrit), 2013-11-05 01:52:31]

I'm not qualified to review this patch but the structure looks good to me and
I'd really like to see it integrated.

[Dmitry Lomov (no reviews), 2013-11-05 13:44:05]

On 2013/11/05 01:52:31, Ken Russell wrote:
> I'm not qualified to review this patch but the structure looks good to me and
> I'd really like to see it integrated.

Second that. There are at least two issues with ArrayBuffers blocked on this
(see the bug)

[kbalazs, 2013-11-05 14:17:48]

The linux bot seems sick:
../../sandbox/linux/services/credentials.cc:8:28: fatalerror: sys/capability.h:
No such file or directory
This is not touched in the patch.

Anyway, I can retry it, but previously it built everywhere besides windows and
now windows is green (and the change I made to fix it should not affect other
platforms), so I think it builds fine now.

[Avi (use Gerrit), 2013-11-05 14:51:35]

On 2013/11/05 14:17:48, kbalazs wrote:
> The linux bot seems sick:
> ../../sandbox/linux/services/credentials.cc:8:28: fatalerror:
sys/capability.h:
> No such file or directory
> This is not touched in the patch.
> 
> Anyway, I can retry it, but previously it built everywhere besides windows and
> now windows is green (and the change I made to fix it should not affect other
> platforms), so I think it builds fine now.

Mac stuff LGTM, tho I can't talk about other platforms.

[alkondratenko, 2013-11-05 18:44:16]

On 2013/11/03 02:04:44, willchan wrote:
> The right way to do this would be to expose a new tcmalloc function.
> tc_malloc_unchecked() or something. And have base call that directly when
we're
> using TCMalloc.

From perspective of upstreaming this change I find tc_malloc_unchecked() name a
bit confusing. Default tc_malloc behavior is already "return NULL on enomem". So
new function basically means "ignore tc_new_mode" and _unchecked doesn't seem to
communicate that at all.

Also it appears that malloc shim is still doing do_malloc call directly, without
using new API.

[Ken Russell (switch to Gerrit), 2013-11-05 19:20:33]

The missing sys/capability.h header file is caused by a missing build dependency
on the bot. It's not your CL. I've notified the troopers.

[kbalazs, 2013-11-05 20:44:40]

On 2013/11/05 18:44:16, alkondratenko wrote:
> On 2013/11/03 02:04:44, willchan wrote:
> > The right way to do this would be to expose a new tcmalloc function.
> > tc_malloc_unchecked() or something. And have base call that directly when
> we're
> > using TCMalloc.
> 
> From perspective of upstreaming this change I find tc_malloc_unchecked() name
a
> bit confusing. Default tc_malloc behavior is already "return NULL on enomem".
So
> new function basically means "ignore tc_new_mode" and _unchecked doesn't seem
to
> communicate that at all.
> 
> Also it appears that malloc shim is still doing do_malloc call directly,
without
> using new API.

The malloc shim already uses do_malloc in it's malloc redefine. It's a tricky
thing, allocator_shim.cc actually includes tcmalloc.cc.

I'm not sure I have a better idea for the name, _unchecked indeed does not
communicate the details but it is not misguiding either. Could you propose
something? tc_malloc_skip_new_handler?

[kbalazs, 2013-11-06 16:39:15]

Seems like only the naming issue is unresolved.

What about tc_default_malloc (default as it does not care about tc_new_mode)?

Anyway it does not seems to be blocker, so I would like to ask a review at this
point.

[alkondratenko, 2013-11-06 22:11:04]

On 2013/11/06 16:39:15, kbalazs wrote:
> Seems like only the naming issue is unresolved.
> 
> What about tc_default_malloc (default as it does not care about tc_new_mode)?
> 
> Anyway it does not seems to be blocker, so I would like to ask a review at
this
> point.

I like tc_malloc_skip_new_handler name better.

[kbalazs, 2013-11-06 22:57:37]

Renamed the new function to tc_malloc_skip_new_handler

[timvolodine1, 2013-11-07 11:25:15]

after doing a gclient sync on Linux checkout I now get:
../../sandbox/linux/services/credentials.cc:9:28: fatal error:
sys/capability.h: No such file or directory

Is this being fixed? Or did something change wrt compilation?


On Wed, Nov 6, 2013 at 10:57 PM, <b.kelemen@samsung.com> wrote:

> Renamed the new function to tc_malloc_skip_new_handler
>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[timvolodine1, 2013-11-07 11:31:58]

ah I see, sudo build/install-build-deps.sh seems to fix the issue.



On Thu, Nov 7, 2013 at 11:25 AM, Tim Volodine <timvolodine@google.com>wrote:

> after doing a gclient sync on Linux checkout I now get:
> ../../sandbox/linux/services/credentials.cc:9:28: fatal error:
> sys/capability.h: No such file or directory
>
> Is this being fixed? Or did something change wrt compilation?
>
>
> On Wed, Nov 6, 2013 at 10:57 PM, <b.kelemen@samsung.com> wrote:
>
>> Renamed the new function to tc_malloc_skip_new_handler
>>
>> https://codereview.chromium.org/55333002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2013-11-07 15:10:02]

Sorry for the reviewing delay. I've been at IETF all week. I'll try to get
to this soon.


On Thu, Nov 7, 2013 at 3:31 AM, Tim Volodine <timvolodine@google.com> wrote:

> ah I see, sudo build/install-build-deps.sh seems to fix the issue.
>
>
>
> On Thu, Nov 7, 2013 at 11:25 AM, Tim Volodine <timvolodine@google.com>wrote:
>
>> after doing a gclient sync on Linux checkout I now get:
>> ../../sandbox/linux/services/credentials.cc:9:28: fatal error:
>> sys/capability.h: No such file or directory
>>
>> Is this being fixed? Or did something change wrt compilation?
>>
>>
>> On Wed, Nov 6, 2013 at 10:57 PM, <b.kelemen@samsung.com> wrote:
>>
>>> Renamed the new function to tc_malloc_skip_new_handler
>>>
>>> https://codereview.chromium.org/55333002/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2013-11-12 03:39:15]

Possible to get a review on this soon?

[willchan no longer on Chromium, 2013-11-12 16:04:19]

https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linu...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linu...
base/process/memory_linux.cc:144: if (alloc_size && (alloc_size / size) !=
num_items)
Please add another set of parentheses around the != condition so people don't
have to remember the operator precedence rules.

What happens when |size| is 0? I don't know what happens for calloc(), but we
should be consistent at least. Is it OK to crash?

https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linu...
base/process/memory_linux.cc:145: return NULL;
Shouldn't this be return false?

[willchan no longer on Chromium, 2013-11-12 16:06:35]

Oh, and please update the CL description explaining the implementation (exposing
a new API in TCMalloc).

[kbalazs, 2013-11-12 18:40:45]

https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linu...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/660001/base/process/memory_linu...
base/process/memory_linux.cc:144: if (alloc_size && (alloc_size / size) !=
num_items)
On 2013/11/12 16:04:19, willchan wrote:
> Please add another set of parentheses around the != condition so people don't
> have to remember the operator precedence rules.
> 
> What happens when |size| is 0? I don't know what happens for calloc(), but we
> should be consistent at least. Is it OK to crash?

I made a mistake, the first part of the condition should be size not alloc_size.
Anyway, I will check what is the semantics of calloc in this regard.

[kbalazs, 2013-11-12 18:41:42]

Updated UncheckedCalloc to be consistent with tcmalloc's calloc, updated cl
description

[kbalazs, 2013-11-13 23:21:52]

Can we finish this?

Eventually it turned out that the Blink folks rather want to go with
WTF::PartitionAlloc for this kind of situation.

Still I think this is worth to unify this thing and supporting it directly in
tcmalloc instead of the current hack that is used now in
SkMemory_new_handler.cpp (which could be cleaned up after this), and there can
be other situations in chromium when it can be useful.

[willchan no longer on Chromium, 2013-11-14 00:16:23]

Sorry, I realize you aren't aware that I'm only part-time employed at
Google. I am at 20% employment and worked all last week at a conference,
hence my slowness here. Unfortunately, I'm also the most qualified to
review this code, which is why I haven't handed it off, as I do with most
of my reviews.


On Wed, Nov 13, 2013 at 3:21 PM, <b.kelemen@samsung.com> wrote:

> Can we finish this?
>
> Eventually it turned out that the Blink folks rather want to go with
> WTF::PartitionAlloc for this kind of situation.
>
> Still I think this is worth to unify this thing and supporting it directly
> in
> tcmalloc instead of the current hack that is used now in
> SkMemory_new_handler.cpp (which could be cleaned up after this), and there
> can
> be other situations in chromium when it can be useful.
>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2013-11-14 00:30:16]

On 2013/11/14 00:16:23, willchan wrote:
> Sorry, I realize you aren't aware that I'm only part-time employed at
> Google. I am at 20% employment and worked all last week at a conference,
> hence my slowness here. Unfortunately, I'm also the most qualified to
> review this code, which is why I haven't handed it off, as I do with most
> of my reviews.
> 
> 

No problem. Sorry if I was harsh.

[willchan no longer on Chromium, 2013-11-14 00:31:25]

lgtm

https://codereview.chromium.org/55333002/diff/760001/third_party/tcmalloc/chr...
File third_party/tcmalloc/chromium/src/debugallocation.cc (right):

https://codereview.chromium.org/55333002/diff/760001/third_party/tcmalloc/chr...
third_party/tcmalloc/chromium/src/debugallocation.cc:1434: extern "C"
PERFTOOLS_DLL_DECL void* tc_malloc_skip_new_handler(size_t size) __THROW {
80 col wrap

[willchan no longer on Chromium, 2013-11-14 00:32:32]

No need to apologize, I appreciate pings. I just realized that you weren't
aware of my situation unless most Google devs, so I figured that I should
inform you.


On Wed, Nov 13, 2013 at 4:31 PM, <willchan@chromium.org> wrote:

> lgtm
>
>
> https://codereview.chromium.org/55333002/diff/760001/
> third_party/tcmalloc/chromium/src/debugallocation.cc
> File third_party/tcmalloc/chromium/src/debugallocation.cc (right):
>
> https://codereview.chromium.org/55333002/diff/760001/
> third_party/tcmalloc/chromium/src/debugallocation.cc#newcode1434
> third_party/tcmalloc/chromium/src/debugallocation.cc:1434: extern "C"
> PERFTOOLS_DLL_DECL void* tc_malloc_skip_new_handler(size_t size) __THROW
> {
> 80 col wrap
>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-11-14 00:48:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/900001

[commit-bot: I haz the power, 2013-11-14 02:04:09]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests,
chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, gpu_unittests, installer_util_unittests,
interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests,
mini_installer_test, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-14 16:39:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1160001

[commit-bot: I haz the power, 2013-11-14 18:20:05]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[kbalazs, 2013-11-15 00:04:59]

I have this issue:
./out/Release/sandbox_linux_unittests: symbol lookup error:
/home/balazs/src/chromium/src/out/Release/lib/libbase.so: undefined symbol:
tc_malloc_skip_new_handler

Both content_shell and chrome works fine.

[kbalazs, 2013-11-15 01:11:53]

I had to add allocator.gyp:allocator as a dependency for
sandbox_linux_unittests. This is somewhat common, grepping through the gyp files
for this dependency has 72 matches. If you think this is wrong please shout.
Otherwise I will retry landing tomorrow.

[commit-bot: I haz the power, 2013-11-15 16:16:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1360001

[commit-bot: I haz the power, 2013-11-15 16:45:42]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Markus (顧孟勤), 2013-11-15 21:15:39]

lgtm

I don't sufficiently understand GYP to say whether this is the best fix. It
looks to me as if this is really a bug with libbase that should be addressed in
that component, rather than in all of its users.

But that might be out of the scope of your CL, and therefore your suggested fix
could very well the best we can do at the time.

So, I hesitantly give you an LGTM -- and I am cc'ing Julien who might have more
insights, as he has wrangled with GYP a lot in the past.

[kbalazs, 2013-11-15 23:27:59]

Yes I think this is a problem with base. It does not depend on allocator. The
reason why there is no issue with that for symbols like tc_malloc is that they
are not called directly, but the default malloc is aliased to tc_malloc. So as
long as a target does not use something that is implemented in base/allocator
(like malloc hooks), it can work without it. I guess in this case the default
malloc is actually not aliased to tc_malloc - which is weird since when tcmalloc
is enabled every target should actually use it imho. Anyway, most executable
targets need to depend on allocator. I think it would be better to make base
depend on it (and remove the dependency from the executable targets). But as you
say it is beyond the scope of this change.

[commit-bot: I haz the power, 2013-11-18 17:48:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1360001

[commit-bot: I haz the power, 2013-11-18 18:20:29]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[kbalazs, 2013-11-18 19:00:11]

There is a seemingly unrelated error on android_clang_dbg:

In file included from
../../third_party/tcmalloc/chromium/src/base/atomicops.h:95:
../../third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h:219:25:error:
value size does not match register size specified by the constraint and modifier
[-Werror,-Wasm-operand-widths]
      : "r" (ptr), "r" (new_value)
 
This builder is green in the waterfall.

[Dai Mikurube (NOT FULLTIME), 2013-11-20 03:51:32]

On 2013/11/18 19:00:11, kbalazs wrote:
> There is a seemingly unrelated error on android_clang_dbg:
> 
> In file included from
> ../../third_party/tcmalloc/chromium/src/base/atomicops.h:95:
>
../../third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h:219:25:error:
> value size does not match register size specified by the constraint and
modifier
> [-Werror,-Wasm-operand-widths]
>       : "r" (ptr), "r" (new_value)
>  
> This builder is green in the waterfall.

Before all, IIRC, Chromium for Android doesn't use tcmalloc by default without
specific build options (for now). Can you check your build options?

I can't see which file includes this atomicops.h. Does the error message include
the information?

[kbalazs, 2013-11-20 18:11:30]

On 2013/11/20 03:51:32, Dai Mikurube wrote:
> On 2013/11/18 19:00:11, kbalazs wrote:
> > There is a seemingly unrelated error on android_clang_dbg:
> > 
> > In file included from
> > ../../third_party/tcmalloc/chromium/src/base/atomicops.h:95:
> >
>
../../third_party/tcmalloc/chromium/src/base/atomicops-internals-arm-v6plus.h:219:25:error:
> > value size does not match register size specified by the constraint and
> modifier
> > [-Werror,-Wasm-operand-widths]
> >       : "r" (ptr), "r" (new_value)
> >  
> > This builder is green in the waterfall.
> 
> Before all, IIRC, Chromium for Android doesn't use tcmalloc by default without
> specific build options (for now). Can you check your build options?
> 
> I can't see which file includes this atomicops.h. Does the error message
include
> the information?

You are absolutely right, tcmalloc is not enabled so the allocator target should
not not be built on Android. The bug with my patch was that I unconditionally
added it as a dependency to sandbox. In fact if you do "ninja allocator" it will
fail on android with clang without the patch.

[commit-bot: I haz the power, 2013-11-20 20:11:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/1740001

[commit-bot: I haz the power, 2013-11-20 21:23:54]

Retried try job too often on linux_chromeos for step(s)
cacheinvalidation_unittests, dbus_unittests, google_apis_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[kbalazs, 2013-11-20 22:58:44]

This will not work. Other unittests are failing with the same error that
sandbox_unittests had before. allocator need to be a dependency of base. I will
make this change and ask for review again.

[kbalazs, 2013-11-21 00:28:45]

Could somebody review it with the latest changes in base.gyp? The rest is the
same.

[kbalazs, 2013-11-25 15:57:45]

@willchan: could you review it again with the latest change in base.gyp?

[willchan no longer on Chromium, 2013-11-27 01:48:27]

Sorry, I thought this CL was done and ignored all updates. I believe adding this
dependency is wrong, as it will add the TCMalloc dependency to anything using
libbase. But some platforms like Android choose not to use TCMalloc. I think the
right thing to do is use our allocator thunk stuff...but I forget. I'll look
into this and get back to you.

[kbalazs, 2013-11-27 17:06:15]

On 2013/11/27 01:48:27, willchan wrote:
> Sorry, I thought this CL was done and ignored all updates. I believe adding
this
> dependency is wrong, as it will add the TCMalloc dependency to anything using
> libbase. But some platforms like Android choose not to use TCMalloc. I think
the
> right thing to do is use our allocator thunk stuff...but I forget. I'll look
> into this and get back to you.

I added the dependency conditionally when tcmalloc is used. Currently this
dependency is repeated in executable targets, with this we could remove that.
Today some targets that do not use malloc thunks could avoid depending on
allocator, but I think this is rather a bug than a feature (I think if we use
tcmalloc than we should use it everywhere, not just in content_shell or chrome
but also in unittests).

I don't think thunks can work for this because thunks are for the reverse
direction, i.e. to allow tcmalloc to call into client code.

[willchan no longer on Chromium, 2013-11-28 03:37:07]

On 2013/11/27 17:06:15, kbalazs wrote:
> On 2013/11/27 01:48:27, willchan wrote:
> > Sorry, I thought this CL was done and ignored all updates. I believe adding
> this
> > dependency is wrong, as it will add the TCMalloc dependency to anything
using
> > libbase. But some platforms like Android choose not to use TCMalloc. I think
> the
> > right thing to do is use our allocator thunk stuff...but I forget. I'll look
> > into this and get back to you.
> 
> I added the dependency conditionally when tcmalloc is used. Currently this
> dependency is repeated in executable targets, with this we could remove that.
> Today some targets that do not use malloc thunks could avoid depending on
> allocator, but I think this is rather a bug than a feature (I think if we use
> tcmalloc than we should use it everywhere, not just in content_shell or chrome
> but also in unittests).

I agree that we should use it in every executable we control. But it's wrong for
a library to dictate the choice of allocator. The library has no knowledge about
how the embedder uses it. For example, the application could be a game with its
own special allocator, and it might link in content to display some web content
every once in awhile. It's wrong for libcontent to have mandated the use of the
allocator. Only the final application knows how it'll be used. So the allocator
dependency should be added in at every executable in the Chromium project,
unless it opts out of the allocator dependency (like Chromium on Android).



> 
> I don't think thunks can work for this because thunks are for the reverse
> direction, i.e. to allow tcmalloc to call into client code.

Thanks for reminding me of that.

[willchan no longer on Chromium, 2013-11-28 03:38:27]

I think your solution is fine for now. It's possible we could use weak symbols
instead to solve this problem, but your solution seems fine too.

LGTM.

https://codereview.chromium.org/55333002/diff/1160001/base/process/memory_lin...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/1160001/base/process/memory_lin...
base/process/memory_linux.cc:14: #include
"third_party/tcmalloc/chromium/src/gperftools/tcmalloc.h"
You should use an #if defined(USE_TCMALLOC) here.

https://codereview.chromium.org/55333002/diff/1160001/base/process/memory_lin...
base/process/memory_linux.cc:136: #else // tcmalloc
please do #elif defined(USE_TCMALLOC) and an #else #error.

[commit-bot: I haz the power, 2013-12-02 16:50:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/2090001

[commit-bot: I haz the power, 2013-12-02 18:36:07]

Retried try job too often on linux_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[kbalazs, 2013-12-02 22:53:06]

On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_rel for step(s) nacl_integration
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Not good.... This makes NaCL tests crash with this:
../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption
detected.

So it seems like NaCL (at least) does not work well with tcmalloc. So the only
workable way is to add the allocator dependency to every executable.

[kbalazs, 2013-12-03 00:32:38]

On 2013/12/02 22:53:06, kbalazs wrote:
> On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_rel for step(s) nacl_integration
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> 
> Not good.... This makes NaCL tests crash with this:
> ../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption
> detected.
> 
> So it seems like NaCL (at least) does not work well with tcmalloc. So the only
> workable way is to add the allocator dependency to every executable.

This again not seems to be a possible solution as there are hundreds of
executables. I'm going to check the nacl tests locally, although I am getting to
be pessimistic about the possibility of landing this change. I welcome ideas, of
course.

[Ken Russell (switch to Gerrit), 2013-12-03 21:53:53]

On 2013/12/03 00:32:38, kbalazs wrote:
> On 2013/12/02 22:53:06, kbalazs wrote:
> > On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote:
> > > Retried try job too often on linux_rel for step(s) nacl_integration
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> > 
> > Not good.... This makes NaCL tests crash with this:
> > ../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption
> > detected.
> > 
> > So it seems like NaCL (at least) does not work well with tcmalloc. So the
only
> > workable way is to add the allocator dependency to every executable.
> 
> This again not seems to be a possible solution as there are hundreds of
> executables. I'm going to check the nacl tests locally, although I am getting
to
> be pessimistic about the possibility of landing this change. I welcome ideas,
of
> course.

Please don't be discouraged. You're the first person with the experience and
persistence to attempt this change in years. I think it is really important to
put this primitive in place.

Have you considered or tried overriding linux_use_tcmalloc=0 in some of the
NaCl-related GYP files?

[kbalazs, 2013-12-04 21:51:20]

Adding nacl owners in the hope that they can help. :)

[kbalazs, 2013-12-04 21:53:59]

On 2013/12/03 21:53:53, Ken Russell wrote:
> On 2013/12/03 00:32:38, kbalazs wrote:
> > On 2013/12/02 22:53:06, kbalazs wrote:
> > > On 2013/12/02 18:36:07, I haz the power (commit-bot) wrote:
> > > > Retried try job too often on linux_rel for step(s) nacl_integration
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
> > > 
> > > Not good.... This makes NaCL tests crash with this:
> > > ../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption
> > > detected.
> > > 
> > > So it seems like NaCL (at least) does not work well with tcmalloc. So the
> only
> > > workable way is to add the allocator dependency to every executable.
> > 
> > This again not seems to be a possible solution as there are hundreds of
> > executables. I'm going to check the nacl tests locally, although I am
getting
> to
> > be pessimistic about the possibility of landing this change. I welcome
ideas,
> of
> > course.
> 
> Please don't be discouraged. You're the first person with the experience and
> persistence to attempt this change in years. I think it is really important to
> put this primitive in place.

Thanks for the encouraging words! :)

 
> Have you considered or tried overriding linux_use_tcmalloc=0 in some of the
> NaCl-related GYP files?

This will not help, once libbase.so is linked to allocator and thus tcmalloc
everything that depends on base will use tcmalloc. Basically variables like this
should only be set in common.gypi.

[kbalazs, 2013-12-04 22:00:20]

On 2013/12/04 21:51:20, kbalazs wrote:
> Adding nacl owners in the hope that they can help. :)

So the situation is that I made base depending on allocator.gyp:allocator, to
avoid adding this dependency to every executable. And it somehow made nacl tests
cause heap corruption:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&....

Error message: ../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory
corruption detected.

I can hardly read the output or understand how a nacl program works. Does that
mean that now nacl programs use tcmalloc and it is a bad thing?

[kbalazs, 2013-12-04 22:03:35]

Btw I could not reproduce this locally.

I used this command: 
python chrome/test/nacl_test_injection/buildbot_chrome_nacl_stage.py
--disable_glibc --disable_tests=run_ppapi_ppb_image_data_browser_test --mode

It seems to be the right thing as the output is similar as on the bot but no
heap corruption message occurs to me.

[bradn, 2013-12-04 22:48:11]

So I'm told by ncbray that this is probably actual memory corruption in chrome
that isn't related to nacl. nacl_integration is the only test suite that
directly starts the chrome executable (as opposed to browser_tests etc). The
lack of a netlog would tend to indicate that corruption happens before any
network prefetching.

PID 13055
../../third_party/tcmalloc/chromium/src/free_list.h:118] Memory corruption
detected. 
[ 30063 ms] |||| 
[ 30063 ms] |||| [SERVER_ERROR] Did not hear from the test for 30.0 seconds.
[ 30063 ms] |||| Heard from Javascript 30.1 seconds ago.
[ 30063 ms] |||| The renderer probably hung or crashed.

No URLs were served by the test runner. It is unlikely this test failure has
anything to do with this particular test.

Cannot find netlog, did Chrome actually launch?
##################### Terminating the browser

You can run the tests more directly in the src/native_client directory with
this:

/usr/bin/python scons.py --verbose platform=x86-64
--mode=opt-host,nacl,nacl_irt_test chrome_browser_path=<path_to_chrome>
chrome_browser_tests

You can run a specific test quickly by running the command that appears in the
output log directly before the test.

[kbalazs, 2013-12-05 01:14:20]

I still cannot reproduce this, both chrome and the nacl tests work fine (Linux,
64 bit). What's wrong with you linux_rel?

[Nick Bray (chromium), 2013-12-05 02:06:52]

On 2013/12/05 01:14:20, kbalazs wrote:
> I still cannot reproduce this, both chrome and the nacl tests work fine
(Linux,
> 64 bit). What's wrong with you linux_rel?

I think this would be worth getting to the bottom of, we've seen chrome crash on
linux during startup in the past.  If need be, I think it would be possible to
log on to the specific bot reporting the error and build/test on that VM.

If it's a real memory corruption bug, I'd imagine it's much more likely to
happen in a 32-bit address space than a 64-bit address space.

It could be non-deterministic and/or dependent on machine state, and linux_rel
just has timings that tickle it.  Have you tried re-rerunning the try job
recently?  A clean run does not solve the problem, but it gives more data.

It could be that some of the flags nacl_integration pass chrome (such as writing
the netlog to disk) tickle a code path that other tests don't.  This path could
have the memory corruption bug.

Does a passing nacl_integration test on Linux 64 also print "Memory corruption
detected." ?  This might indicate we're conflating two unrelated issues.

[kbalazs, 2013-12-05 16:24:20]

On 2013/12/05 02:06:52, Nick Bray (chromium) wrote:
> On 2013/12/05 01:14:20, kbalazs wrote:
> > I still cannot reproduce this, both chrome and the nacl tests work fine
> (Linux,
> > 64 bit). What's wrong with you linux_rel?
> 
> I think this would be worth getting to the bottom of, we've seen chrome crash
on
> linux during startup in the past.  If need be, I think it would be possible to
> log on to the specific bot reporting the error and build/test on that VM.
> 
> If it's a real memory corruption bug, I'd imagine it's much more likely to
> happen in a 32-bit address space than a 64-bit address space.
> 
> It could be non-deterministic and/or dependent on machine state, and linux_rel
> just has timings that tickle it.  Have you tried re-rerunning the try job
> recently?  A clean run does not solve the problem, but it gives more data.
> 
> It could be that some of the flags nacl_integration pass chrome (such as
writing
> the netlog to disk) tickle a code path that other tests don't.  This path
could
> have the memory corruption bug.
> 
> Does a passing nacl_integration test on Linux 64 also print "Memory corruption
> detected." ?  This might indicate we're conflating two unrelated issues.

I did not see the memory corruption message locally. I'm gonna retry the try job
with a clean run and test it locally in 32 bit.

[kbalazs, 2013-12-06 00:26:01]

The tests do not really work well in my 32 bit build but not failing with the
same error. If I rerun the command thus opening the test page manually it does
not crash. I did cross-compile, not 32 bit, should I try a 32 bit chroot or vm?
Btw from the logs linux_rel do not seem like a 32 bit machine, is it?

Do you generally think that this change (i.e. forcing every binary to use
tcmalloc when it is turned on) can be harmful for native client? Is there
anything special with the memory management of nacl programs? How malloc works
in an nexe? If it should use the same malloc as the browser than I see no reason
what can be wrong with this change, so I was thinking that probably there is
something special about that.

[kbalazs, 2013-12-12 23:10:21]

I have no clue what to do with this. If someone could log in to this bot and
grab a backtrace it would be extremely useful.

[willchan no longer on Chromium, 2013-12-12 23:12:42]

Sorry I haven't been around, I'm only part-time. I don't have an
answer here for you, but I would like you to keep working on this (I
recognize it's cheap for me to say this, but your time is valuable). I
recommend asking for help on the IRC channel.

On Thu, Dec 12, 2013 at 3:10 PM,  <b.kelemen@samsung.com> wrote:
> I have no clue what to do with this. If someone could log in to this bot and
> grab a backtrace it would be extremely useful.
>
> https://codereview.chromium.org/55333002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2013-12-13 18:08:43]

nacl_integration was green on linux_rel_preciese32. It does not give too much
information but at least proves that bug is kind of flaky.

[Nico, 2014-01-08 03:20:13]

https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocato...
File base/allocator/allocator_shim.cc (right):

https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocato...
base/allocator/allocator_shim.cc:388: *result = je_malloc(size);
Don't these need break statements?

[kbalazs, 2014-01-13 19:46:28]

On 2014/01/08 03:20:13, Nico wrote:
>
https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocato...
> File base/allocator/allocator_shim.cc (right):
> 
>
https://codereview.chromium.org/55333002/diff/2110001/base/allocator/allocato...
> base/allocator/allocator_shim.cc:388: *result = je_malloc(size);
> Don't these need break statements?

Oops, yes they need.

[kbalazs, 2014-01-13 20:00:39]

I was able to reproduce this. It is component=static_library that makes a
difference. And the problem is that libppGoogleNaClPluginChrome.so and chrome
both contains a copy of tcmalloc (with the patch) and something weird happens
when the plugin dlopened. I'm not sure what is it exactly but anyway it's wrong
that tcmalloc is duplicated. Btw it looks like libppGoogleNaClPluginChrome only
depends on base via media.gyp:shared_memory_support. So maybe I could devide
base.gyp:base to a part that is ok to be used from an so and make base.gyp:base
contain that plus the allocator dependency. Of course adding the allocator
dependency to executables is a cleaner approach, I'm just scared because there
are a lot.

[kbalazs, 2014-01-13 22:57:55]

I was so blind... All we need is an indirection across base and tcmalloc. I can
push the function pointer to base from higher level that already depends on
tcmalloc. Patch will come.

[kbalazs, 2014-01-13 23:47:29]

On 2014/01/13 22:57:55, kbalazs wrote:
> I was so blind... All we need is an indirection across base and tcmalloc. I
can
> push the function pointer to base from higher level that already depends on
> tcmalloc. Patch will come.

Done. Please review again.

[Ken Russell (switch to Gerrit), 2014-01-14 07:07:53]

On 2014/01/13 23:47:29, kbalazs wrote:
> On 2014/01/13 22:57:55, kbalazs wrote:
> > I was so blind... All we need is an indirection across base and tcmalloc. I
> can
> > push the function pointer to base from higher level that already depends on
> > tcmalloc. Patch will come.
> 
> Done. Please review again.

Thank you for persisting with this and tracking down the issue!

I'm not qualified to review this patch, but would still like to see it land.

[willchan no longer on Chromium, 2014-01-15 00:25:04]

Can you update the changelist description? I don't understand what your latest
changes are doing.

And I don't think the pepper plugin should be building with TCMalloc. And even
if it is, all the symbols should be hidden, so if you get a link error, that's a
build configuration bug.

[kbalazs, 2014-01-15 21:37:38]

On 2014/01/15 00:25:04, willchan wrote:
> Can you update the changelist description? I don't understand what your latest
> changes are doing.

Ok, I will update the description with the technical details. The concept is the
same generally. The difference is that I don't try to use tcmalloc directly in
base. Instead I bind base::UncheckedMalloc with tc_malloc_skip_new_handler in a
layer when I can use tcmalloc, in ContentMainRunner.  In case of unit tests that
do not go through ContentMainRunner that binding does not happen of course and
in this case base::UncheckedMalloc will just call malloc.

> And I don't think the pepper plugin should be building with TCMalloc. And even
> if it is, all the symbols should be hidden, so if you get a link error, that's
a
> build configuration bug.

Right, the plugin doesn't link to tcmalloc now, it was only the case with the
previous approach when base linked to tcmalloc.

[kbalazs, 2014-01-15 22:24:56]

Something weird happens on linux_chromeos:

python ../../../scripts/slave/runtest.py --target Release ...
...
Traceback (most recent call last):
  File "../../../scripts/slave/runtest.py", line 1557, in <module>
    sys.exit(main())
  File "../../../scripts/slave/runtest.py", line 1529, in main
    result = main_linux(options, args)
  File "../../../scripts/slave/runtest.py", line 915, in main_linux
    raise chromium_utils.PathNotFound(msg)
common.chromium_utils.PathNotFound: Unable to find
/mnt/scratch0/b_used/build/slave/linux_chromeos/build/src/out/Release/accessibility_unittests_br

There is no target accessibility_unittests_br for me locally either, there is
only accessibility_unittests. This runtest.py does not lives in the tree so I
can't check further how it gets confused.

[Nico, 2014-01-15 22:34:52]

On Wed, Jan 15, 2014 at 2:24 PM, <b.kelemen@samsung.com> wrote:

> Something weird happens on linux_chromeos:
>
> python ../../../scripts/slave/runtest.py --target Release ...
> ...
> Traceback (most recent call last):
>   File "../../../scripts/slave/runtest.py", line 1557, in <module>
>     sys.exit(main())
>   File "../../../scripts/slave/runtest.py", line 1529, in main
>     result = main_linux(options, args)
>   File "../../../scripts/slave/runtest.py", line 915, in main_linux
>     raise chromium_utils.PathNotFound(msg)
> common.chromium_utils.PathNotFound: Unable to find
> /mnt/scratch0/b_used/build/slave/linux_chromeos/build/
> src/out/Release/accessibility_unittests_br
>
> There is no target accessibility_unittests_br for me locally either, there
> is
> only accessibility_unittests. This runtest.py does not lives in the tree
> so I
> can't check further how it gets confused.
>

It's in the tree, just not in the part folks usually check out – the script
is only needed on bots. It's here:
https://code.google.com/p/chromium/codesearch#search/&q=runtest.py&sq=package...

…but it'll probably resolve itself in a few hours, so maybe just wait.


>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-15 22:36:50]

On Wed, Jan 15, 2014 at 2:34 PM, Nico Weber <thakis@chromium.org> wrote:

> On Wed, Jan 15, 2014 at 2:24 PM, <b.kelemen@samsung.com> wrote:
>
>> Something weird happens on linux_chromeos:
>>
>> python ../../../scripts/slave/runtest.py --target Release ...
>> ...
>> Traceback (most recent call last):
>>   File "../../../scripts/slave/runtest.py", line 1557, in <module>
>>     sys.exit(main())
>>   File "../../../scripts/slave/runtest.py", line 1529, in main
>>     result = main_linux(options, args)
>>   File "../../../scripts/slave/runtest.py", line 915, in main_linux
>>     raise chromium_utils.PathNotFound(msg)
>> common.chromium_utils.PathNotFound: Unable to find
>> /mnt/scratch0/b_used/build/slave/linux_chromeos/build/
>> src/out/Release/accessibility_unittests_br
>>
>> There is no target accessibility_unittests_br for me locally either,
>> there is
>> only accessibility_unittests. This runtest.py does not lives in the tree
>> so I
>> can't check further how it gets confused.
>>
>
> It's in the tree, just not in the part folks usually check out – the
> script is only needed on bots. It's here:
>
https://code.google.com/p/chromium/codesearch#search/&q=runtest.py&sq=package...
>
> …but it'll probably resolve itself in a few hours, so maybe just wait.
>

Using the "annotate" button in cs.chromium.org for
"accessibility_unittests_br" tells me that this is a known problem (
https://codereview.chromium.org/107413007#msg8) and that a fix is on the
way:
https://chromiumcodereview.appspot.com/135153004/diff/1/scripts/master/factor...


>
>
>>
>> https://codereview.chromium.org/55333002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-01-15 22:51:15]

> >
> > It's in the tree, just not in the part folks usually check out – the
> > script is only needed on bots. It's here:
> >
>
https://code.google.com/p/chromium/codesearch#search/&q=runtest.py&sq=package...
> >
> > …but it'll probably resolve itself in a few hours, so maybe just wait.
> >
> 
> Using the "annotate" button in http://cs.chromium.org for
> "accessibility_unittests_br" tells me that this is a known problem (
> https://codereview.chromium.org/107413007#msg8) and that a fix is on the
> way:
>
https://chromiumcodereview.appspot.com/135153004/diff/1/scripts/master/factor...
> 

Thanks for finding it out.

[willchan no longer on Chromium, 2014-01-16 01:30:03]

I fixed your CL desc to say "link" instead of "use" TCMalloc directly. Since
it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
definition.

Will there be another change to call this delegate function in skia somewhere?

I'm also toying with the idea of using a static initializer in allocator_shim
here. In that case, we're already paying the hit of the static initializers for
starting up TCMalloc, so maybe we should just hook into that for providing this
UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
initialize the skip handler in base for TCMalloc.

Nico, any thoughts on a static initializer for this specific case? I understand
we're against it in general (which I support), but I suspect if we bundle this
with existing TCMalloc static initializers, there's no net increase in cost. But
there would be a simplicity win, since now we don't have to initialize the
delegate function in every app/unittest.

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
File base/process/memory.h (right):

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
base/process/memory.h:74: // FIXME: make Skia use the new interface and remove
these.
Nit: Please use TODO(username). Doesn't mean you have to do it, but if someone
needs someone to ask about this, you're a good person to ask.

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function function)
{
Please CHECK(!tc_malloc_skip_new_handler); It should only be called once.

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void**
result) {
I'm confused in this function. Why don't we always just use malloc()? tc_malloc
overrides the malloc() weak symbol. And libc's malloc provides the malloc weak
symbol, so it should be safe to call malloc() there too instead of
__libc_malloc.

AFAICT, the only conditional needed is the USE_TCMALLOC so you know whether or
not to check skipping the new handler.

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
base/process/memory_linux.cc:152: #error Not implemented
I don't believe it's possible to ever hit this #else. You have:

#if ((foo) || (!A && !B))
...
#elif (A && !B)
...
#elif (B)
...
#else
...
#endif

https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
File content/app/content_main_runner.cc (left):

https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
defined(USE_TCMALLOC)
Thanks for the cleanup.

[Nico, 2014-01-16 01:52:17]

On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:

> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
> Since
> it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
> definition.
>
> Will there be another change to call this delegate function in skia
> somewhere?
>
> I'm also toying with the idea of using a static initializer in
> allocator_shim
> here. In that case, we're already paying the hit of the static
> initializers for
> starting up TCMalloc, so maybe we should just hook into that for providing
> this
> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
> initialize the skip handler in base for TCMalloc.


> Nico, any thoughts on a static initializer for this specific case? I
> understand
> we're against it in general (which I support), but I suspect if we bundle
> this
> with existing TCMalloc static initializers, there's no net increase in
> cost. But
> there would be a simplicity win, since now we don't have to initialize the
> delegate function in every app/unittest.
>

Dunno, we want to be at 0 initializers at this point, so I'd rather not
make that harder.


>
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory.h
> File base/process/memory.h (right):
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory.h#newcode74
> base/process/memory.h:74: // FIXME: make Skia use the new interface and
> remove these.
> Nit: Please use TODO(username). Doesn't mean you have to do it, but if
> someone needs someone to ask about this, you're a good person to ask.
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory_linux.cc
> File base/process/memory_linux.cc (right):
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory_linux.cc#newcode136
> base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function
> function) {
> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
> once.
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory_linux.cc#newcode141
> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
> void** result) {
> I'm confused in this function. Why don't we always just use malloc()?
> tc_malloc overrides the malloc() weak symbol. And libc's malloc provides
> the malloc weak symbol, so it should be safe to call malloc() there too
> instead of __libc_malloc.
>
> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
> whether or not to check skipping the new handler.
>
> https://codereview.chromium.org/55333002/diff/2370001/
> base/process/memory_linux.cc#newcode152
> base/process/memory_linux.cc:152: #error Not implemented
> I don't believe it's possible to ever hit this #else. You have:
>
> #if ((foo) || (!A && !B))
> ...
> #elif (A && !B)
> ...
> #elif (B)
> ...
> #else
> ...
> #endif
>
> https://codereview.chromium.org/55333002/diff/2370001/
> content/app/content_main_runner.cc
> File content/app/content_main_runner.cc (left):
>
> https://codereview.chromium.org/55333002/diff/2370001/
> content/app/content_main_runner.cc#oldcode98
> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
> defined(USE_TCMALLOC)
> Thanks for the cleanup.
>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-16 01:52:28]

On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:

> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
>
>> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
>> Since
>> it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
>> definition.
>>
>> Will there be another change to call this delegate function in skia
>> somewhere?
>>
>> I'm also toying with the idea of using a static initializer in
>> allocator_shim
>> here. In that case, we're already paying the hit of the static
>> initializers for
>> starting up TCMalloc, so maybe we should just hook into that for
>> providing this
>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
>> initialize the skip handler in base for TCMalloc.
>
>
>> Nico, any thoughts on a static initializer for this specific case? I
>> understand
>> we're against it in general (which I support), but I suspect if we bundle
>> this
>> with existing TCMalloc static initializers, there's no net increase in
>> cost. But
>> there would be a simplicity win, since now we don't have to initialize the
>> delegate function in every app/unittest.
>>
>
> Dunno, we want to be at 0 initializers at this point, so I'd rather not
> make that harder.
>

*at _some_ point


>
>
>>
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory.h
>> File base/process/memory.h (right):
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory.h#newcode74
>> base/process/memory.h:74: // FIXME: make Skia use the new interface and
>> remove these.
>> Nit: Please use TODO(username). Doesn't mean you have to do it, but if
>> someone needs someone to ask about this, you're a good person to ask.
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory_linux.cc
>> File base/process/memory_linux.cc (right):
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory_linux.cc#newcode136
>> base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function
>> function) {
>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
>> once.
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory_linux.cc#newcode141
>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
>> void** result) {
>> I'm confused in this function. Why don't we always just use malloc()?
>> tc_malloc overrides the malloc() weak symbol. And libc's malloc provides
>> the malloc weak symbol, so it should be safe to call malloc() there too
>> instead of __libc_malloc.
>>
>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
>> whether or not to check skipping the new handler.
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> base/process/memory_linux.cc#newcode152
>> base/process/memory_linux.cc:152: #error Not implemented
>> I don't believe it's possible to ever hit this #else. You have:
>>
>> #if ((foo) || (!A && !B))
>> ...
>> #elif (A && !B)
>> ...
>> #elif (B)
>> ...
>> #else
>> ...
>> #endif
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> content/app/content_main_runner.cc
>> File content/app/content_main_runner.cc (left):
>>
>> https://codereview.chromium.org/55333002/diff/2370001/
>> content/app/content_main_runner.cc#oldcode98
>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
>> defined(USE_TCMALLOC)
>> Thanks for the cleanup.
>>
>> https://codereview.chromium.org/55333002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-01-16 01:58:03]

On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
>>
>> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
>>>
>>> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
>>> Since
>>> it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
>>> definition.
>>>
>>> Will there be another change to call this delegate function in skia
>>> somewhere?
>>>
>>> I'm also toying with the idea of using a static initializer in
>>> allocator_shim
>>> here. In that case, we're already paying the hit of the static
>>> initializers for
>>> starting up TCMalloc, so maybe we should just hook into that for
>>> providing this
>>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
>>> initialize the skip handler in base for TCMalloc.
>>>
>>>
>>> Nico, any thoughts on a static initializer for this specific case? I
>>> understand
>>> we're against it in general (which I support), but I suspect if we bundle
>>> this
>>> with existing TCMalloc static initializers, there's no net increase in
>>> cost. But
>>> there would be a simplicity win, since now we don't have to initialize
>>> the
>>> delegate function in every app/unittest.
>>
>>
>> Dunno, we want to be at 0 initializers at this point, so I'd rather not
>> make that harder.
>
>
> *at _some_ point

While I agree with you in terms of absolutism (static initializers are
bad), I'm trying to balance tradeoffs here. I assert the marginal perf
cost is negligible (especially if the static initializer gets bundled
with the existing TCMalloc initializers), and the marginal increase in
difficulty of removing the static initializers from TCMalloc is quite
small. I also assert that the maintainability improvement here (not
having to set the delegate function in all the various places) is big
enough to offset those costs. These are just opinions in evaluating
the tradeoff, and I'm curious if you evaluate them differently.

>
>>
>>
>>>
>>>
>>>
>>>
>>> https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
>>> File base/process/memory.h (right):
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
>>> base/process/memory.h:74: // FIXME: make Skia use the new interface and
>>> remove these.
>>> Nit: Please use TODO(username). Doesn't mean you have to do it, but if
>>> someone needs someone to ask about this, you're a good person to ask.
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>>> File base/process/memory_linux.cc (right):
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>>> base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function
>>> function) {
>>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
>>> once.
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
>>> void** result) {
>>> I'm confused in this function. Why don't we always just use malloc()?
>>> tc_malloc overrides the malloc() weak symbol. And libc's malloc provides
>>> the malloc weak symbol, so it should be safe to call malloc() there too
>>> instead of __libc_malloc.
>>>
>>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
>>> whether or not to check skipping the new handler.
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>>> base/process/memory_linux.cc:152: #error Not implemented
>>> I don't believe it's possible to ever hit this #else. You have:
>>>
>>> #if ((foo) || (!A && !B))
>>> ...
>>> #elif (A && !B)
>>> ...
>>> #elif (B)
>>> ...
>>> #else
>>> ...
>>> #endif
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>>> File content/app/content_main_runner.cc (left):
>>>
>>>
>>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
>>> defined(USE_TCMALLOC)
>>> Thanks for the cleanup.
>>>
>>> https://codereview.chromium.org/55333002/
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-16 02:08:38]

After reading the patch, can tc_malloc_skip_new_handler in memory_linux.cc
be every something different from tc_malloc_skip_new_handler in tcmalloc.h?


On Wed, Jan 15, 2014 at 5:58 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
> > On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
> >>
> >> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
> >>>
> >>> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
> >>> Since
> >>> it's definitely using TCMalloc, it even has the USE_TCMALLOC
> preprocessor
> >>> definition.
> >>>
> >>> Will there be another change to call this delegate function in skia
> >>> somewhere?
> >>>
> >>> I'm also toying with the idea of using a static initializer in
> >>> allocator_shim
> >>> here. In that case, we're already paying the hit of the static
> >>> initializers for
> >>> starting up TCMalloc, so maybe we should just hook into that for
> >>> providing this
> >>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
> >>> initialize the skip handler in base for TCMalloc.
> >>>
> >>>
> >>> Nico, any thoughts on a static initializer for this specific case? I
> >>> understand
> >>> we're against it in general (which I support), but I suspect if we
> bundle
> >>> this
> >>> with existing TCMalloc static initializers, there's no net increase in
> >>> cost. But
> >>> there would be a simplicity win, since now we don't have to initialize
> >>> the
> >>> delegate function in every app/unittest.
> >>
> >>
> >> Dunno, we want to be at 0 initializers at this point, so I'd rather not
> >> make that harder.
> >
> >
> > *at _some_ point
>
> While I agree with you in terms of absolutism (static initializers are
> bad), I'm trying to balance tradeoffs here. I assert the marginal perf
> cost is negligible (especially if the static initializer gets bundled
> with the existing TCMalloc initializers), and the marginal increase in
> difficulty of removing the static initializers from TCMalloc is quite
> small. I also assert that the maintainability improvement here (not
> having to set the delegate function in all the various places) is big
> enough to offset those costs. These are just opinions in evaluating
> the tradeoff, and I'm curious if you evaluate them differently.
>
> >
> >>
> >>
> >>>
> >>>
> >>>
> >>>
> >>>
> https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
> >>> File base/process/memory.h (right):
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
> >>> base/process/memory.h:74: // FIXME: make Skia use the new interface and
> >>> remove these.
> >>> Nit: Please use TODO(username). Doesn't mean you have to do it, but if
> >>> someone needs someone to ask about this, you're a good person to ask.
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >>> File base/process/memory_linux.cc (right):
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >>> base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function
> >>> function) {
> >>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
> >>> once.
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
> >>> void** result) {
> >>> I'm confused in this function. Why don't we always just use malloc()?
> >>> tc_malloc overrides the malloc() weak symbol. And libc's malloc
> provides
> >>> the malloc weak symbol, so it should be safe to call malloc() there too
> >>> instead of __libc_malloc.
> >>>
> >>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
> >>> whether or not to check skipping the new handler.
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >>> base/process/memory_linux.cc:152: #error Not implemented
> >>> I don't believe it's possible to ever hit this #else. You have:
> >>>
> >>> #if ((foo) || (!A && !B))
> >>> ...
> >>> #elif (A && !B)
> >>> ...
> >>> #elif (B)
> >>> ...
> >>> #else
> >>> ...
> >>> #endif
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
> >>> File content/app/content_main_runner.cc (left):
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
> >>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
> >>> defined(USE_TCMALLOC)
> >>> Thanks for the cleanup.
> >>>
> >>> https://codereview.chromium.org/55333002/
> >>
> >>
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-01-16 02:11:50]

It theoretically could, but we have no plans in practice to ever do that.

The only reason to have the layer of indirection is so that libbase
does not depend on tcmalloc directly. Briefly, the reason to do that
is only the application should be selecting the allocator, not the
lower level library.

On Wed, Jan 15, 2014 at 6:08 PM, Nico Weber <thakis@chromium.org> wrote:
> After reading the patch, can tc_malloc_skip_new_handler in memory_linux.cc
> be every something different from tc_malloc_skip_new_handler in tcmalloc.h?
>
>
> On Wed, Jan 15, 2014 at 5:58 PM, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
>> > On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org> wrote:
>> >>
>> >> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
>> >>>
>> >>> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
>> >>> Since
>> >>> it's definitely using TCMalloc, it even has the USE_TCMALLOC
>> >>> preprocessor
>> >>> definition.
>> >>>
>> >>> Will there be another change to call this delegate function in skia
>> >>> somewhere?
>> >>>
>> >>> I'm also toying with the idea of using a static initializer in
>> >>> allocator_shim
>> >>> here. In that case, we're already paying the hit of the static
>> >>> initializers for
>> >>> starting up TCMalloc, so maybe we should just hook into that for
>> >>> providing this
>> >>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
>> >>> initialize the skip handler in base for TCMalloc.
>> >>>
>> >>>
>> >>> Nico, any thoughts on a static initializer for this specific case? I
>> >>> understand
>> >>> we're against it in general (which I support), but I suspect if we
>> >>> bundle
>> >>> this
>> >>> with existing TCMalloc static initializers, there's no net increase in
>> >>> cost. But
>> >>> there would be a simplicity win, since now we don't have to initialize
>> >>> the
>> >>> delegate function in every app/unittest.
>> >>
>> >>
>> >> Dunno, we want to be at 0 initializers at this point, so I'd rather not
>> >> make that harder.
>> >
>> >
>> > *at _some_ point
>>
>> While I agree with you in terms of absolutism (static initializers are
>> bad), I'm trying to balance tradeoffs here. I assert the marginal perf
>> cost is negligible (especially if the static initializer gets bundled
>> with the existing TCMalloc initializers), and the marginal increase in
>> difficulty of removing the static initializers from TCMalloc is quite
>> small. I also assert that the maintainability improvement here (not
>> having to set the delegate function in all the various places) is big
>> enough to offset those costs. These are just opinions in evaluating
>> the tradeoff, and I'm curious if you evaluate them differently.
>>
>> >
>> >>
>> >>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
>> >>> File base/process/memory.h (right):
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
>> >>> base/process/memory.h:74: // FIXME: make Skia use the new interface
>> >>> and
>> >>> remove these.
>> >>> Nit: Please use TODO(username). Doesn't mean you have to do it, but if
>> >>> someone needs someone to ask about this, you're a good person to ask.
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >>> File base/process/memory_linux.cc (right):
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >>> base/process/memory_linux.cc:136: tc_malloc_skip_new_handler_function
>> >>> function) {
>> >>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
>> >>> once.
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
>> >>> void** result) {
>> >>> I'm confused in this function. Why don't we always just use malloc()?
>> >>> tc_malloc overrides the malloc() weak symbol. And libc's malloc
>> >>> provides
>> >>> the malloc weak symbol, so it should be safe to call malloc() there
>> >>> too
>> >>> instead of __libc_malloc.
>> >>>
>> >>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
>> >>> whether or not to check skipping the new handler.
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >>> base/process/memory_linux.cc:152: #error Not implemented
>> >>> I don't believe it's possible to ever hit this #else. You have:
>> >>>
>> >>> #if ((foo) || (!A && !B))
>> >>> ...
>> >>> #elif (A && !B)
>> >>> ...
>> >>> #elif (B)
>> >>> ...
>> >>> #else
>> >>> ...
>> >>> #endif
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>> >>> File content/app/content_main_runner.cc (left):
>> >>>
>> >>>
>> >>>
>> >>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>> >>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
>> >>> defined(USE_TCMALLOC)
>> >>> Thanks for the cleanup.
>> >>>
>> >>> https://codereview.chromium.org/55333002/
>> >>
>> >>
>> >
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2014-01-16 02:12:59]

On Wed, Jan 15, 2014 at 6:11 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> It theoretically could, but we have no plans in practice to ever do that.
>
> The only reason to have the layer of indirection is so that libbase
> does not depend on tcmalloc directly. Briefly, the reason to do that
> is only the application should be selecting the allocator, not the
> lower level library.
>

How does the application do that? Wouldn't setting this wherever the
allocator is selected by the natural place?


>
> On Wed, Jan 15, 2014 at 6:08 PM, Nico Weber <thakis@chromium.org> wrote:
> > After reading the patch, can tc_malloc_skip_new_handler in
> memory_linux.cc
> > be every something different from tc_malloc_skip_new_handler in
> tcmalloc.h?
> >
> >
> > On Wed, Jan 15, 2014 at 5:58 PM, William Chan (陈智昌) <
> willchan@chromium.org>
> > wrote:
> >>
> >> On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org>
> wrote:
> >> > On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org>
> wrote:
> >> >>
> >> >> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
> >> >>>
> >> >>> I fixed your CL desc to say "link" instead of "use" TCMalloc
> directly.
> >> >>> Since
> >> >>> it's definitely using TCMalloc, it even has the USE_TCMALLOC
> >> >>> preprocessor
> >> >>> definition.
> >> >>>
> >> >>> Will there be another change to call this delegate function in skia
> >> >>> somewhere?
> >> >>>
> >> >>> I'm also toying with the idea of using a static initializer in
> >> >>> allocator_shim
> >> >>> here. In that case, we're already paying the hit of the static
> >> >>> initializers for
> >> >>> starting up TCMalloc, so maybe we should just hook into that for
> >> >>> providing this
> >> >>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as
> well
> >> >>> initialize the skip handler in base for TCMalloc.
> >> >>>
> >> >>>
> >> >>> Nico, any thoughts on a static initializer for this specific case? I
> >> >>> understand
> >> >>> we're against it in general (which I support), but I suspect if we
> >> >>> bundle
> >> >>> this
> >> >>> with existing TCMalloc static initializers, there's no net increase
> in
> >> >>> cost. But
> >> >>> there would be a simplicity win, since now we don't have to
> initialize
> >> >>> the
> >> >>> delegate function in every app/unittest.
> >> >>
> >> >>
> >> >> Dunno, we want to be at 0 initializers at this point, so I'd rather
> not
> >> >> make that harder.
> >> >
> >> >
> >> > *at _some_ point
> >>
> >> While I agree with you in terms of absolutism (static initializers are
> >> bad), I'm trying to balance tradeoffs here. I assert the marginal perf
> >> cost is negligible (especially if the static initializer gets bundled
> >> with the existing TCMalloc initializers), and the marginal increase in
> >> difficulty of removing the static initializers from TCMalloc is quite
> >> small. I also assert that the maintainability improvement here (not
> >> having to set the delegate function in all the various places) is big
> >> enough to offset those costs. These are just opinions in evaluating
> >> the tradeoff, and I'm curious if you evaluate them differently.
> >>
> >> >
> >> >>
> >> >>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> >> >>>
> https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
> >> >>> File base/process/memory.h (right):
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
> >> >>> base/process/memory.h:74: // FIXME: make Skia use the new interface
> >> >>> and
> >> >>> remove these.
> >> >>> Nit: Please use TODO(username). Doesn't mean you have to do it, but
> if
> >> >>> someone needs someone to ask about this, you're a good person to
> ask.
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >> >>> File base/process/memory_linux.cc (right):
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >> >>> base/process/memory_linux.cc:136:
> tc_malloc_skip_new_handler_function
> >> >>> function) {
> >> >>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
> >> >>> once.
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >> >>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
> >> >>> void** result) {
> >> >>> I'm confused in this function. Why don't we always just use
> malloc()?
> >> >>> tc_malloc overrides the malloc() weak symbol. And libc's malloc
> >> >>> provides
> >> >>> the malloc weak symbol, so it should be safe to call malloc() there
> >> >>> too
> >> >>> instead of __libc_malloc.
> >> >>>
> >> >>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
> >> >>> whether or not to check skipping the new handler.
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> >> >>> base/process/memory_linux.cc:152: #error Not implemented
> >> >>> I don't believe it's possible to ever hit this #else. You have:
> >> >>>
> >> >>> #if ((foo) || (!A && !B))
> >> >>> ...
> >> >>> #elif (A && !B)
> >> >>> ...
> >> >>> #elif (B)
> >> >>> ...
> >> >>> #else
> >> >>> ...
> >> >>> #endif
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
> >> >>> File content/app/content_main_runner.cc (left):
> >> >>>
> >> >>>
> >> >>>
> >> >>>
>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
> >> >>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
> >> >>> defined(USE_TCMALLOC)
> >> >>> Thanks for the cleanup.
> >> >>>
> >> >>> https://codereview.chromium.org/55333002/
> >> >>
> >> >>
> >> >
> >
> >
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-01-16 02:24:48]

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void**
result) {
In case of LIBC_GLIBC && !USE_TCMALLOC we override malloc in this file to die on
oom. So we need to call the original one to be able to not die.

[willchan no longer on Chromium, 2014-01-16 02:37:19]

On Wed, Jan 15, 2014 at 6:12 PM, Nico Weber <thakis@chromium.org> wrote:
> On Wed, Jan 15, 2014 at 6:11 PM, William Chan (陈智昌) <willchan@chromium.org>
> wrote:
>>
>> It theoretically could, but we have no plans in practice to ever do that.
>>
>> The only reason to have the layer of indirection is so that libbase
>> does not depend on tcmalloc directly. Briefly, the reason to do that
>> is only the application should be selecting the allocator, not the
>> lower level library.
>
>
> How does the application do that? Wouldn't setting this wherever the
> allocator is selected by the natural place?

By adding a link-time dependency. Weak symbols are overridden and the
first entrance into the memory allocator lazily initializes. There's
also a static initializer that initializes, but it usually no-ops
since some other static initializer usually runs first and invokes
malloc.

>
>>
>>
>> On Wed, Jan 15, 2014 at 6:08 PM, Nico Weber <thakis@chromium.org> wrote:
>> > After reading the patch, can tc_malloc_skip_new_handler in
>> > memory_linux.cc
>> > be every something different from tc_malloc_skip_new_handler in
>> > tcmalloc.h?
>> >
>> >
>> > On Wed, Jan 15, 2014 at 5:58 PM, William Chan (陈智昌)
>> > <willchan@chromium.org>
>> > wrote:
>> >>
>> >> On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org>
>> >> wrote:
>> >> > On Wed, Jan 15, 2014 at 5:52 PM, Nico Weber <thakis@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Jan 15, 2014 at 5:30 PM, <willchan@chromium.org> wrote:
>> >> >>>
>> >> >>> I fixed your CL desc to say "link" instead of "use" TCMalloc
>> >> >>> directly.
>> >> >>> Since
>> >> >>> it's definitely using TCMalloc, it even has the USE_TCMALLOC
>> >> >>> preprocessor
>> >> >>> definition.
>> >> >>>
>> >> >>> Will there be another change to call this delegate function in skia
>> >> >>> somewhere?
>> >> >>>
>> >> >>> I'm also toying with the idea of using a static initializer in
>> >> >>> allocator_shim
>> >> >>> here. In that case, we're already paying the hit of the static
>> >> >>> initializers for
>> >> >>> starting up TCMalloc, so maybe we should just hook into that for
>> >> >>> providing this
>> >> >>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as
>> >> >>> well
>> >> >>> initialize the skip handler in base for TCMalloc.
>> >> >>>
>> >> >>>
>> >> >>> Nico, any thoughts on a static initializer for this specific case?
>> >> >>> I
>> >> >>> understand
>> >> >>> we're against it in general (which I support), but I suspect if we
>> >> >>> bundle
>> >> >>> this
>> >> >>> with existing TCMalloc static initializers, there's no net increase
>> >> >>> in
>> >> >>> cost. But
>> >> >>> there would be a simplicity win, since now we don't have to
>> >> >>> initialize
>> >> >>> the
>> >> >>> delegate function in every app/unittest.
>> >> >>
>> >> >>
>> >> >> Dunno, we want to be at 0 initializers at this point, so I'd rather
>> >> >> not
>> >> >> make that harder.
>> >> >
>> >> >
>> >> > *at _some_ point
>> >>
>> >> While I agree with you in terms of absolutism (static initializers are
>> >> bad), I'm trying to balance tradeoffs here. I assert the marginal perf
>> >> cost is negligible (especially if the static initializer gets bundled
>> >> with the existing TCMalloc initializers), and the marginal increase in
>> >> difficulty of removing the static initializers from TCMalloc is quite
>> >> small. I also assert that the maintainability improvement here (not
>> >> having to set the delegate function in all the various places) is big
>> >> enough to offset those costs. These are just opinions in evaluating
>> >> the tradeoff, and I'm curious if you evaluate them differently.
>> >>
>> >> >
>> >> >>
>> >> >>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
>> >> >>> File base/process/memory.h (right):
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
>> >> >>> base/process/memory.h:74: // FIXME: make Skia use the new interface
>> >> >>> and
>> >> >>> remove these.
>> >> >>> Nit: Please use TODO(username). Doesn't mean you have to do it, but
>> >> >>> if
>> >> >>> someone needs someone to ask about this, you're a good person to
>> >> >>> ask.
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >> >>> File base/process/memory_linux.cc (right):
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >> >>> base/process/memory_linux.cc:136:
>> >> >>> tc_malloc_skip_new_handler_function
>> >> >>> function) {
>> >> >>> Please CHECK(!tc_malloc_skip_new_handler); It should only be called
>> >> >>> once.
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >> >>> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size,
>> >> >>> void** result) {
>> >> >>> I'm confused in this function. Why don't we always just use
>> >> >>> malloc()?
>> >> >>> tc_malloc overrides the malloc() weak symbol. And libc's malloc
>> >> >>> provides
>> >> >>> the malloc weak symbol, so it should be safe to call malloc() there
>> >> >>> too
>> >> >>> instead of __libc_malloc.
>> >> >>>
>> >> >>> AFAICT, the only conditional needed is the USE_TCMALLOC so you know
>> >> >>> whether or not to check skipping the new handler.
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
>> >> >>> base/process/memory_linux.cc:152: #error Not implemented
>> >> >>> I don't believe it's possible to ever hit this #else. You have:
>> >> >>>
>> >> >>> #if ((foo) || (!A && !B))
>> >> >>> ...
>> >> >>> #elif (A && !B)
>> >> >>> ...
>> >> >>> #elif (B)
>> >> >>> ...
>> >> >>> #else
>> >> >>> ...
>> >> >>> #endif
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>> >> >>> File content/app/content_main_runner.cc (left):
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai...
>> >> >>> content/app/content_main_runner.cc:98: #if !defined(OS_MACOSX) &&
>> >> >>> defined(USE_TCMALLOC)
>> >> >>> Thanks for the cleanup.
>> >> >>>
>> >> >>> https://codereview.chromium.org/55333002/
>> >> >>
>> >> >>
>> >> >
>> >
>> >
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-01-16 02:47:12]

On 2014/01/16 02:12:59, Nico wrote:
> On Wed, Jan 15, 2014 at 6:11 PM, William Chan (陈智昌)
> <willchan@chromium.org>wrote:
> 
> > It theoretically could, but we have no plans in practice to ever do that.
> >
> > The only reason to have the layer of indirection is so that libbase
> > does not depend on tcmalloc directly. Briefly, the reason to do that
> > is only the application should be selecting the allocator, not the
> > lower level library.
> >
> 
> How does the application do that? Wouldn't setting this wherever the
> allocator is selected by the natural place?
> 

This is confusing. USE_TCMALLOC is defined in base on Linux, but it does not
mean that base can call tcmalloc exposed functions. The allocator has to be
added as dependency in gyp to make the target actually link to tcmalloc. The
reason this work like that is that tcmalloc does not expose the standard malloc
functions but overrides them. So there is not much code that actually needs to
call something that tcmalloc exposes.

So it is really the final target that selects the allocator. We cannot add the
dependency as low as base because even things like the ppapi plugin depend on
base. In a static build this means that we would have 2 copy of it after loading
the plugin (which actually brakes but even if it would work it's bad).

But maybe we can add it to content, since ContentMainRunner already calls
tcmalloc exposed functions. I guess today it only works because every target
that depend on content links to tcmalloc (in configurations when USE_TCMALLOC is
defined).

[kbalazs, 2014-01-16 02:53:04]

> But maybe we can add it to content, since ContentMainRunner already calls
> tcmalloc exposed functions. I guess today it only works because every target
> that depend on content links to tcmalloc (in configurations when USE_TCMALLOC
is
> defined).

Ah, forget about that part, I realized that it is not a solution for this
problem because we want to add UncheckedMalloc to base so we would still need
the indirection or the magic with static initializers.

[willchan no longer on Chromium, 2014-01-16 02:54:42]

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void**
result) {
On 2014/01/16 02:24:50, kbalazs wrote:
> In case of LIBC_GLIBC && !USE_TCMALLOC we override malloc in this file to die
on
> oom. So we need to call the original one to be able to not die.

I see. That's a good point...I'm trying to think up an alternative, but I don't
know of one. You and I understand these issues now, but this code is pretty
unreadable to anyone else :) Would be great if we could improve this somehow...

[kbalazs, 2014-01-16 03:11:40]

On 2014/01/16 01:30:03, willchan wrote:
> I fixed your CL desc to say "link" instead of "use" TCMalloc directly. Since
> it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
> definition.
> 
> Will there be another change to call this delegate function in skia somewhere?

Yes. Also there are problems in WebGL when a size of a buffer comes from js and
can be arbitrary big. crbug.com/283716 is an example.

> 
> I'm also toying with the idea of using a static initializer in allocator_shim
> here. In that case, we're already paying the hit of the static initializers
for
> starting up TCMalloc, so maybe we should just hook into that for providing
this
> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
> initialize the skip handler in base for TCMalloc.

I was also thinking about something like that after looking at TCMallocGuard but
I don't think that can work. How can the static initializer set up base for
using the right function? Since tcmalloc and base cannot depend on each other,
it seems to me that the only doable solution is to hook them together elsewhere.

[willchan no longer on Chromium, 2014-01-16 03:14:15]

On Wed, Jan 15, 2014 at 7:11 PM,  <b.kelemen@samsung.com> wrote:
> On 2014/01/16 01:30:03, willchan wrote:
>>
>> I fixed your CL desc to say "link" instead of "use" TCMalloc directly.
>> Since
>> it's definitely using TCMalloc, it even has the USE_TCMALLOC preprocessor
>> definition.
>
>
>> Will there be another change to call this delegate function in skia
>> somewhere?
>
>
> Yes. Also there are problems in WebGL when a size of a buffer comes from js
> and
> can be arbitrary big. crbug.com/283716 is an example.
>
>
>
>> I'm also toying with the idea of using a static initializer in
>> allocator_shim
>> here. In that case, we're already paying the hit of the static
>> initializers
>
> for
>>
>> starting up TCMalloc, so maybe we should just hook into that for providing
>
> this
>>
>> UncheckedMalloc. Since if we are initializing TCMalloc, we may as well
>> initialize the skip handler in base for TCMalloc.
>
>
> I was also thinking about something like that after looking at TCMallocGuard
> but
> I don't think that can work. How can the static initializer set up base for
> using the right function? Since tcmalloc and base cannot depend on each
> other,
> it seems to me that the only doable solution is to hook them together
> elsewhere.

allocator is not part of libbase, yet it's in the base/ directory. So
it may be reasonable for it to reach into libbase to initialize
members.

>
>
> https://codereview.chromium.org/55333002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-01-16 03:24:56]

On 2014/01/16 02:54:42, willchan wrote:
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> File base/process/memory_linux.cc (right):
> 
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory_lin...
> base/process/memory_linux.cc:141: bool UncheckedMalloc(size_t size, void**
> result) {
> On 2014/01/16 02:24:50, kbalazs wrote:
> > In case of LIBC_GLIBC && !USE_TCMALLOC we override malloc in this file to
die
> on
> > oom. So we need to call the original one to be able to not die.
> 
> I see. That's a good point...I'm trying to think up an alternative, but I
don't
> know of one. You and I understand these issues now, but this code is pretty
> unreadable to anyone else :) Would be great if we could improve this
somehow...

Well, at least it is much easier to read with context. Reading through
memory_linux.cc it is more clear why __libc_malloc needs to be called here.

[kbalazs, 2014-01-16 03:46:41]

> allocator is not part of libbase, yet it's in the base/ directory. So
> it may be reasonable for it to reach into libbase to initialize
> members. 

I would say the directory layout is a bug since base do not contain allocator
and cannot even link to it. But yes, it seems to be doable to use base in the
allocator.

If I understand you correctly that the static initializer we have now for
tcmalloc is not really good for anything since there is no guarantee that malloc
will not be called earlier by another static initializer. So we could remove it
right now, couldn't we? If so then relying on it to solve this problem would
make the static initializer problem worst.

[kbalazs, 2014-01-17 18:01:16]

@willchan: so, should I update the patch, or wait for you with the static
initializer thing? :)

[willchan no longer on Chromium, 2014-01-28 22:03:43]

On 2014/01/17 18:01:16, kbalazs wrote:
> @willchan: so, should I update the patch, or wait for you with the static
> initializer thing? :)

Let's punt on the static initializer, I have no time to work on this.

My main issue with this code is what's going on in
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai....
It feels fragile. How are we supposed to test Blink without the content harness?
Are you going to add this skip new handler everywhere?

[jln (very slow on Chromium), 2014-01-29 01:34:33]

I didn't read the whole thread, but new (nothrow) wasn't enough to satisfy this
need?

I think it works on all platforms except maybe IOS.

What will be the users of this?

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
File base/process/memory.h (right):

https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
base/process/memory.h:67: // set to NULL, otherwise it holds the memory address.
Could you add a comment along the lines of: "Please don't use this unless
absolutely needed. For security reasons the return value must always be checked"
?

[kbalazs, 2014-01-29 16:28:35]

On 2014/01/29 01:34:33, jln wrote:
> I didn't read the whole thread, but new (nothrow) wasn't enough to satisfy
this
> need?

Almost, but it is not thread safe. See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...

> 
> I think it works on all platforms except maybe IOS.
> 
> What will be the users of this?

First I will change the nothrow use in skia for this
(https://code.google.com/p/chromium/codesearch#chromium/src/skia/ext/SkMemory_...).

Here is another one where I think it will help: crbug.com/283716. I'm pretty
sure there are more.

> 
> https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h
> File base/process/memory.h (right):
> 
>
https://codereview.chromium.org/55333002/diff/2370001/base/process/memory.h#n...
> base/process/memory.h:67: // set to NULL, otherwise it holds the memory
address.
> Could you add a comment along the lines of: "Please don't use this unless
> absolutely needed. For security reasons the return value must always be
checked"
> ?

WARN_UNUSED_RETURN is documenting it for some extent already, but sure, I can
add this as well.

[kbalazs, 2014-01-29 16:35:40]

On 2014/01/28 22:03:43, willchan wrote:
> On 2014/01/17 18:01:16, kbalazs wrote:
> > @willchan: so, should I update the patch, or wait for you with the static
> > initializer thing? :)
> 
> Let's punt on the static initializer, I have no time to work on this.
> 
> My main issue with this code is what's going on in
>
https://codereview.chromium.org/55333002/diff/2370001/content/app/content_mai....
> It feels fragile. How are we supposed to test Blink without the content
harness?
> Are you going to add this skip new handler everywhere?

No, I don't want to add it for every unit test and the like (targets that ends
in a binary), that is just too much bloat (adding dependency to allocator for
all of them). That is why I need to set it up in runtime. In case it is not set
up the UncheckedMalloc will not be really unchecked (just falls back to malloc).
Does it look ok to you with this in mind?

[kbalazs, 2014-02-04 19:24:01]

I don't see any real issues on the try bots. PTAL

[willchan no longer on Chromium, 2014-02-04 19:45:50]

jln: Did that comment satisfy you?

b.keleman: My concern is that the behavioral differences here are non-obvious
and any debugging that is necessary in this area will be difficult for normal
application developers to debug. I expect very few Chromium engineers to
understand why the UncheckedMalloc() would crash. That's why I think it'd be
nice (ignoring the costs of doing so) to tie this to the build time dependency
on allocator, since that build-time dependency implicitly brings in the
dependency on TCMalloc. It eliminates several permutations in the test matrix
which eases maintenance burden.

[kbalazs, 2014-02-04 21:02:31]

On 2014/02/04 19:45:50, willchan wrote:
> jln: Did that comment satisfy you?
> 
> b.keleman: My concern is that the behavioral differences here are non-obvious
> and any debugging that is necessary in this area will be difficult for normal
> application developers to debug. I expect very few Chromium engineers to
> understand why the UncheckedMalloc() would crash. That's why I think it'd be
> nice (ignoring the costs of doing so) to tie this to the build time dependency
> on allocator, since that build-time dependency implicitly brings in the
> dependency on TCMalloc. It eliminates several permutations in the test matrix
> which eases maintenance burden.

I agree but I don't think it is doable. Once I started the process of adding the
dependency to every binary target but I abandoned, there are tons. Also it would
add too much noise to the build. I think we only have 2 option here: living
together with the introduced complexity or abandon this.

[jln (very slow on Chromium), 2014-02-04 22:03:56]

On 2014/02/04 19:45:50, willchan wrote:
> jln: Did that comment satisfy you?

Yes, the comment looks good. Thanks!

[Ken Russell (switch to Gerrit), 2014-02-05 00:29:01]

On 2014/02/04 21:02:31, kbalazs wrote:
> 
> I agree but I don't think it is doable. Once I started the process of adding
the
> dependency to every binary target but I abandoned, there are tons. Also it
would
> add too much noise to the build. I think we only have 2 option here: living
> together with the introduced complexity or abandon this.

Comment from the sidelines: I'd like to see this CL land. Even though it might
have surprising behavior in some of Blink's unit test harnesses, those tests
don't typically stress OOM behavior.

This longstanding issue of Blink's tryFastMalloc always crashing caused several
problems implementing typed arrays over the past few years. Even though there is
now a different solution for these problems (PartitionAlloc and its ability to
soft-fail allocations), I still think this feature is worth implementing in base
and tcmalloc.

[kbalazs, 2014-02-12 00:08:32]

Ping. Someone should make a decision :)

[Ken Russell (switch to Gerrit), 2014-02-12 00:11:52]

willchan@ told me in the hallway last week that he wanted to get this CL in. :)

[willchan no longer on Chromium, 2014-02-12 00:11:59]

Sorry, I dropped the ball here. I think we should definitely add
support for this. I'm just struggling on the best way to move forward
on it. Let me take another look and get back to you today.

On Tue, Feb 11, 2014 at 4:08 PM,  <b.kelemen@samsung.com> wrote:
> Ping. Someone should make a decision :)
>
> https://codereview.chromium.org/55333002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[willchan no longer on Chromium, 2014-02-12 19:26:16]

nooooo, i didn't press the "publish" button yesterday and lost all my comments
when my chromeos machine restarted. argh. sorry i didn't send this out earlier.

so i chatted with jln, and we prefer to simply do what tcmalloc does and use
weak symbols (yes it's ugly, but it's the least ugly option in our opinions).

summary:
* on linux, tcmalloc injects itself by defining strong symbols that override the
weak symbols for malloc defined by glibc -
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
* the malloc interface doesn't provide an unchecked malloc for obvious reasons
* therefore, in chromium base, we should extend the malloc interface and
_weakly_ define our own unchecked malloc.
* in chromium's tcmalloc layer (probably allocator_shim), we should strongly
define this symbol.

yes, it's a layering violation, but it's the least ugly in our opinions.

[kbalazs, 2014-02-13 00:03:02]

On 2014/02/12 19:26:16, willchan wrote:
> nooooo, i didn't press the "publish" button yesterday and lost all my comments
> when my chromeos machine restarted. argh. sorry i didn't send this out
earlier.
> 
> so i chatted with jln, and we prefer to simply do what tcmalloc does and use
> weak symbols (yes it's ugly, but it's the least ugly option in our opinions).
> 
> summary:
> * on linux, tcmalloc injects itself by defining strong symbols that override
the
> weak symbols for malloc defined by glibc -
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
> * the malloc interface doesn't provide an unchecked malloc for obvious reasons
> * therefore, in chromium base, we should extend the malloc interface and
> _weakly_ define our own unchecked malloc.
> * in chromium's tcmalloc layer (probably allocator_shim), we should strongly
> define this symbol.
> 
> yes, it's a layering violation, but it's the least ugly in our opinions.

Ok, let's do that. allocator_shim is windows only, I will have to implement the
strong symbol directly in our tcmalloc fork for Linux.

[kbalazs, 2014-02-13 00:55:26]

On 2014/02/13 00:03:02, kbalazs wrote:
> On 2014/02/12 19:26:16, willchan wrote:
> > nooooo, i didn't press the "publish" button yesterday and lost all my
comments
> > when my chromeos machine restarted. argh. sorry i didn't send this out
> earlier.
> > 
> > so i chatted with jln, and we prefer to simply do what tcmalloc does and use
> > weak symbols (yes it's ugly, but it's the least ugly option in our
opinions).
> > 
> > summary:
> > * on linux, tcmalloc injects itself by defining strong symbols that override
> the
> > weak symbols for malloc defined by glibc -
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
> > * the malloc interface doesn't provide an unchecked malloc for obvious
reasons
> > * therefore, in chromium base, we should extend the malloc interface and
> > _weakly_ define our own unchecked malloc.
> > * in chromium's tcmalloc layer (probably allocator_shim), we should strongly
> > define this symbol.
> > 
> > yes, it's a layering violation, but it's the least ugly in our opinions.
> 
> Ok, let's do that. allocator_shim is windows only, I will have to implement
the
> strong symbol directly in our tcmalloc fork for Linux.

Ah, and again I realized the problem when started to implement....
That's not gonna work because weak symbols are not supported on windows afaik.
And again the allocator shim which is used on windows is part of liballocator
and linking every target to liballocator is undoable.

[willchan no longer on Chromium, 2014-02-13 01:21:18]

Sorry, I forgot that you wanted to support this on Windows too. Why
not use the same post-build script that we do for TCMalloc on Windows?
IIRC, it strips out the symbols from libcmt and then links in TCMalloc
in its place. And we should already have the relevant gyp dependencies
set up for this. So you can probably just add the same symbol
stripping and addition in the same place.

On Wed, Feb 12, 2014 at 4:55 PM,  <b.kelemen@samsung.com> wrote:
> On 2014/02/13 00:03:02, kbalazs wrote:
>>
>> On 2014/02/12 19:26:16, willchan wrote:
>> > nooooo, i didn't press the "publish" button yesterday and lost all my
>
> comments
>>
>> > when my chromeos machine restarted. argh. sorry i didn't send this out
>> earlier.
>> >
>> > so i chatted with jln, and we prefer to simply do what tcmalloc does and
>> > use
>> > weak symbols (yes it's ugly, but it's the least ugly option in our
>
> opinions).
>>
>> >
>> > summary:
>> > * on linux, tcmalloc injects itself by defining strong symbols that
>> > override
>> the
>> > weak symbols for malloc defined by glibc -
>> >
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/tcmall...
>>
>> > * the malloc interface doesn't provide an unchecked malloc for obvious
>
> reasons
>>
>> > * therefore, in chromium base, we should extend the malloc interface and
>> > _weakly_ define our own unchecked malloc.
>> > * in chromium's tcmalloc layer (probably allocator_shim), we should
>> > strongly
>> > define this symbol.
>> >
>> > yes, it's a layering violation, but it's the least ugly in our opinions.
>
>
>> Ok, let's do that. allocator_shim is windows only, I will have to
>> implement
>
> the
>>
>> strong symbol directly in our tcmalloc fork for Linux.
>
>
> Ah, and again I realized the problem when started to implement....
> That's not gonna work because weak symbols are not supported on windows
> afaik.
> And again the allocator shim which is used on windows is part of
> liballocator
> and linking every target to liballocator is undoable.
>
>
> https://codereview.chromium.org/55333002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-02-13 18:23:51]

On 2014/02/13 01:21:18, willchan wrote:
> Sorry, I forgot that you wanted to support this on Windows too.

Yes, I don't think it would be very useful if not supported on every platform.

> Why
> not use the same post-build script that we do for TCMalloc on Windows?
> IIRC, it strips out the symbols from libcmt and then links in TCMalloc
> in its place. And we should already have the relevant gyp dependencies
> set up for this. So you can probably just add the same symbol
> stripping and addition in the same place.

I did not know about that thing :) Where can I find the script? Am I right that
it replaces malloc and friends to the symbols defined in allocator_shim? Is that
happening for targets that don't add allocator as a dependency?

[willchan no longer on Chromium, 2014-02-13 21:02:59]

On 2014/02/13 18:23:51, kbalazs wrote:
> On 2014/02/13 01:21:18, willchan wrote:
> > Sorry, I forgot that you wanted to support this on Windows too.
> 
> Yes, I don't think it would be very useful if not supported on every platform.

Just to be clear though, we support this allocator stuff differently on each
platform, so not working for Windows shouldn't necessarily block a Linux
specific solution which could be checked in separately.

> 
> > Why
> > not use the same post-build script that we do for TCMalloc on Windows?
> > IIRC, it strips out the symbols from libcmt and then links in TCMalloc
> > in its place. And we should already have the relevant gyp dependencies
> > set up for this. So you can probably just add the same symbol
> > stripping and addition in the same place.
> 
> I did not know about that thing :) Where can I find the script? Am I right
that
> it replaces malloc and friends to the symbols defined in allocator_shim? Is
that
> happening for targets that don't add allocator as a dependency?

https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/pre...

[kbalazs, 2014-02-13 22:18:22]

> > 
> > I did not know about that thing :) Where can I find the script? Am I right
> that
> > it replaces malloc and friends to the symbols defined in allocator_shim? Is
> that
> > happening for targets that don't add allocator as a dependency?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/pre...

I see. How is that related with the stuff in patch_functions.cc? I guess the
latter is the one that patches the call sites, so the script itself is not
enough. Also we cannot do something like removing the symbol from libbase so the
linker will find it from liballocator because that does not work in static build
(whereas libcrt is always a dll). I am right now trying to hack something up for
windows, will see how it goes.

[willchan no longer on Chromium, 2014-02-13 22:24:38]

On 2014/02/13 22:18:22, kbalazs wrote:
> > > 
> > > I did not know about that thing :) Where can I find the script? Am I right
> > that
> > > it replaces malloc and friends to the symbols defined in allocator_shim?
Is
> > that
> > > happening for targets that don't add allocator as a dependency?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/allocator/pre...
> 
> I see. How is that related with the stuff in patch_functions.cc? I guess the
> latter is the one that patches the call sites, so the script itself is not
> enough. Also we cannot do something like removing the symbol from libbase so
the
> linker will find it from liballocator because that does not work in static
build
> (whereas libcrt is always a dll). I am right now trying to hack something up
for
> windows, will see how it goes.

We don't use the google-perftools function patching strategy as we found it to
be unreliable in comparison to completely stripping the malloc symbols out so we
could replace them with ours. jar@ is the expert on how we do this for Windows.
I know the Linux side.

[kbalazs, 2014-02-13 22:49:30]

> We don't use the google-perftools function patching strategy as we found it to
> be unreliable in comparison to completely stripping the malloc symbols out so
we
> could replace them with ours. jar@ is the expert on how we do this for
Windows.
> I know the Linux side.

@jar: could you explain to me how that works? What I can think about is this: if
a target depend on allocator than you create a special libcrt for it where
malloc and friends are removed. Is that right? Is that need to be done for
everything that links to liballocator? Do you think it is possible to do
something similar with a symbol defined in base? (i.e. removing it from libbase
if allocator is also linked to the target thus simulating a weak symbol in base
that is defined in allocator.)

For being honest I must say that this thing start to look a bit too complex to
me. Surely with native weak symbols it would be a bit better than my approach of
hooking it up in runtime, but if we even have to simulate weak symbols with our
build system...?

[willchan no longer on Chromium, 2014-02-13 22:57:00]

Even if you get blocked here on the Windows side of the solution, I'd
urge you to finish up the Linux side and land that. Incremental
progress is better than none. But I hope you and jar@ can figure out
how to get this to work for Windows.

On Thu, Feb 13, 2014 at 2:49 PM,  <b.kelemen@samsung.com> wrote:
>> We don't use the google-perftools function patching strategy as we found
>> it to
>> be unreliable in comparison to completely stripping the malloc symbols out
>> so
>
> we
>>
>> could replace them with ours. jar@ is the expert on how we do this for
>
> Windows.
>>
>> I know the Linux side.
>
>
> @jar: could you explain to me how that works? What I can think about is
> this: if
> a target depend on allocator than you create a special libcrt for it where
> malloc and friends are removed. Is that right? Is that need to be done for
> everything that links to liballocator? Do you think it is possible to do
> something similar with a symbol defined in base? (i.e. removing it from
> libbase
> if allocator is also linked to the target thus simulating a weak symbol in
> base
> that is defined in allocator.)
>
> For being honest I must say that this thing start to look a bit too complex
> to
> me. Surely with native weak symbols it would be a bit better than my
> approach of
> hooking it up in runtime, but if we even have to simulate weak symbols with
> our
> build system...?
>
> https://codereview.chromium.org/55333002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-02-14 20:55:17]

On 2014/02/13 22:57:00, willchan wrote:
> Even if you get blocked here on the Windows side of the solution, I'd
> urge you to finish up the Linux side and land that. Incremental
> progress is better than none. But I hope you and jar@ can figure out
> how to get this to work for Windows.
 
I can go for the Linux implementation but I still don't think it's a good idea.
I understand that it is feasible for a system library but it is feasible for
libbase? I'm not sure.

[kbalazs, 2014-02-14 22:23:03]

Uploaded the weak symbol version, will update description if we agree this is
the way to go.

[jar (doing other things), 2014-02-15 00:37:43]

On 2014/02/13 22:57:00, willchan wrote:
> Even if you get blocked here on the Windows side of the solution, I'd
> urge you to finish up the Linux side and land that. Incremental
> progress is better than none. But I hope you and jar@ can figure out
> how to get this to work for Windows.
> 
> On Thu, Feb 13, 2014 at 2:49 PM,  <mailto:b.kelemen@samsung.com> wrote:
> >> We don't use the google-perftools function patching strategy as we found
> >> it to
> >> be unreliable in comparison to completely stripping the malloc symbols out
> >> so
> >
> > we
> >>
> >> could replace them with ours. jar@ is the expert on how we do this for
> >
> > Windows.
> >>
> >> I know the Linux side.
> >
> >
> > @jar: could you explain to me how that works? What I can think about is
> > this: if
> > a target depend on allocator than you create a special libcrt for it where
> > malloc and friends are removed. Is that right? Is that need to be done for
> > everything that links to liballocator? Do you think it is possible to do
> > something similar with a symbol defined in base? (i.e. removing it from
> > libbase
> > if allocator is also linked to the target thus simulating a weak symbol in
> > base
> > that is defined in allocator.)
> >
> > For being honest I must say that this thing start to look a bit too complex
> > to
> > me. Surely with native weak symbols it would be a bit better than my
> > approach of
> > hooking it up in runtime, but if we even have to simulate weak symbols with
> > our
> > build system...?
> >
> > https://codereview.chromium.org/55333002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Glancing at the code... and thinking back.... when TCMalloc fails in malloc it
then calls into a global new-handler (if it exists).  If you want to avoid that
handling (a process abort in Chromium), you have to write parallel code that
doesn't test (and use) this global handler.  Once you write that code (sadly...
it will probably be replicated code... although it is plausible that you could
add a state arg at each level of the call stack without harming perf), you
should then plumb the calls down through the shim, and write something for the
other non-TCMalloc implementations at the shim level.

Most of the plumbing sure seems cross-platform, no matter how it is done.  The
only thing "special" for windows is plumbing through the shim (I think).

What is the confusing issue? What am I missing?

[kbalazs, 2014-02-15 01:15:28]

> Glancing at the code... and thinking back.... when TCMalloc fails in malloc it
> then calls into a global new-handler (if it exists).  If you want to avoid
that
> handling (a process abort in Chromium), you have to write parallel code that
> doesn't test (and use) this global handler.  Once you write that code
(sadly...
> it will probably be replicated code... although it is plausible that you could
> add a state arg at each level of the call stack without harming perf), you
> should then plumb the calls down through the shim, and write something for the
> other non-TCMalloc implementations at the shim level.
> 
> Most of the plumbing sure seems cross-platform, no matter how it is done.  The
> only thing "special" for windows is plumbing through the shim (I think).
> 
> What is the confusing issue? What am I missing?

The new handler is a global variable so it won'g work well with threads. When
you open the gate, it will be open for every thread. What I am trying to add is
a malloc primitive that skips the oom handler for it's caller hence allows the
caller to handler gracefully if the allocation fails. But just that particular
caller. For this I need a function that just allocates memory and don't care
about the new handler. In case of tcmalloc this function is do_malloc.

So the naive idea: let's define a new public api on tcmalloc that just calls
that and use it in base. The problem is that base cannot depend on tcmalloc
directly. So we need a trick to call a tcmalloc exposed function from base. A
weak symbol would be good for that, the only problem is that windows doesn't
support weak symbols. So we need to simulate them. For malloc and friends this
is already done today by throwing away some symbols from libcrt.dll. @willchan
proposes to do something similar for this case to simulate a weak symbol in
base. Do you think that's feasible?

[jar (doing other things), 2014-02-26 03:59:50]

The big thing you need is test code.

I think there were several large bugs... and code like this has to be perfect.

Better API is also suggested (see below).

https://codereview.chromium.org/55333002/diff/3040001/base/process/memory_win.cc
File base/process/memory_win.cc (right):

https://codereview.chromium.org/55333002/diff/3040001/base/process/memory_win...
base/process/memory_win.cc:88: // weak symbols are not supported no Windows this
will involve some build time
typo: "no Windows" -->  "on Windows"

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory.cc
File base/process/memory.cc (right):

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory.cc#...
base/process/memory.cc:17: result = NULL;
Should this be:

*result = NULL;

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory.cc#...
base/process/memory.cc:24: memset(result, 0, alloc_size);
Should this be:
memset(*result, 0, alloc_size);

Is there any test code for this module?

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_lin...
File base/process/memory_linux.cc (right):

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_lin...
base/process/memory_linux.cc:205: return *result;
nit: Clearer might be:
return NULL != *result;

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_mac...
base/process/memory_mac.mm:530: return *result;
nit: Clearer might be:
return NULL != *result;

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_mac...
base/process/memory_mac.mm:535: bool result ALLOW_UNUSED = UncheckedMalloc(size,
&address);
It is a bit distressing to see both a bool return, and a NULL vs non-NULL
return.  Replication of results leads to bugs IMO.

IMO, better to have one source of truth... and it sure seems ilke the pointer is
a safer thing to use consistently.

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_win.cc
File base/process/memory_win.cc (right):

https://codereview.chromium.org/55333002/diff/3140001/base/process/memory_win...
base/process/memory_win.cc:93: return !!*result;
nit: Clearer might be:
return NULL != *result;

...or as mentioned in the other comment... perhaps you should return void, and
count on the address being set / cleared.

[Scott Hess - ex-Googler, 2014-02-26 04:16:44]

I think the use of a boolean return with the pointer by reference was
because at some point I suggested it as a way to force WARN_UNUSED_RESULT.
 It is cumbersome relative to standard malloc().  The entire thing is
cumbersome relative to standard malloc()!  But I only feel that it's a good
idea to somehow encourage people to really check things in this case,
rather than just storing the pointer, I don't consider it essential.

[I also no longer really recall if there was discussion or if I was just
assumed to have authority.]

-scott


On Tue, Feb 25, 2014 at 7:59 PM, <jar@chromium.org> wrote:

> The big thing you need is test code.
>
> I think there were several large bugs... and code like this has to be
> perfect.
>
> Better API is also suggested (see below).
>
>
> https://codereview.chromium.org/55333002/diff/3040001/
> base/process/memory_win.cc
> File base/process/memory_win.cc (right):
>
> https://codereview.chromium.org/55333002/diff/3040001/
> base/process/memory_win.cc#newcode88
> base/process/memory_win.cc:88: // weak symbols are not supported no
> Windows this will involve some build time
> typo: "no Windows" -->  "on Windows"
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory.cc
> File base/process/memory.cc (right):
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory.cc#newcode17
> base/process/memory.cc:17: result = NULL;
> Should this be:
>
> *result = NULL;
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory.cc#newcode24
> base/process/memory.cc:24: memset(result, 0, alloc_size);
> Should this be:
> memset(*result, 0, alloc_size);
>
> Is there any test code for this module?
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_linux.cc
> File base/process/memory_linux.cc (right):
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_linux.cc#newcode205
> base/process/memory_linux.cc:205: return *result;
> nit: Clearer might be:
> return NULL != *result;
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_mac.mm
> File base/process/memory_mac.mm (right):
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_mac.mm#newcode530
> base/process/memory_mac.mm:530: return *result;
> nit: Clearer might be:
> return NULL != *result;
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_mac.mm#newcode535
> base/process/memory_mac.mm:535: bool result ALLOW_UNUSED =
> UncheckedMalloc(size, &address);
> It is a bit distressing to see both a bool return, and a NULL vs
> non-NULL return.  Replication of results leads to bugs IMO.
>
> IMO, better to have one source of truth... and it sure seems ilke the
> pointer is a safer thing to use consistently.
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_win.cc
> File base/process/memory_win.cc (right):
>
> https://codereview.chromium.org/55333002/diff/3140001/
> base/process/memory_win.cc#newcode93
> base/process/memory_win.cc:93: return !!*result;
> nit: Clearer might be:
> return NULL != *result;
>
> ...or as mentioned in the other comment... perhaps you should return
> void, and count on the address being set / cleared.
>
> https://codereview.chromium.org/55333002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kbalazs, 2014-02-27 15:42:33]

On 2014/02/26 04:16:44, shess wrote:
> I think the use of a boolean return with the pointer by reference was
> because at some point I suggested it as a way to force WARN_UNUSED_RESULT.
>  It is cumbersome relative to standard malloc().  The entire thing is
> cumbersome relative to standard malloc()!  But I only feel that it's a good
> idea to somehow encourage people to really check things in this case,
> rather than just storing the pointer, I don't consider it essential.
> 
> [I also no longer really recall if there was discussion or if I was just
> assumed to have authority.]
 
I don't have a really strong preference but I think forcing WARN_UNUSED_RESULT
is a good idea so I would stick to that form.

[kbalazs, 2014-03-06 20:09:07]

Added unit test. PTAL.

[kbalazs, 2014-03-06 20:09:48]

On 2014/03/06 20:09:07, kbalazs wrote:
> Added unit test. PTAL.

Will update commit message later... if you like the patch.

[kbalazs, 2014-03-13 22:12:13]

Could someone take a look?

[Scott Hess - ex-Googler, 2014-03-13 22:30:31]

Just reviewed the test, I assume everything else is rebasing noise.

[Might be worth posting the rebase separately in the future, so that it's easy
to find the material change.]

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:386: SetUpInDeathAssert();
This shouldn't be a death test, right?

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:388: EXPECT_TRUE(value_ != NULL);
When these succeed, you'll need to free value_.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:394: SetUpInDeathAssert();
Also not a death test?

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:399: EXPECT_EQ(memcmp(value_, zero, 512), 0);
I think prefer a kConstant to copies of 512.

I think the obvious for() loop would be clearer than crafting a buffer to
memcmp().

[kbalazs, 2014-03-13 23:06:28]

On 2014/03/13 22:30:31, shess wrote:
> Just reviewed the test, I assume everything else is rebasing noise.
> 
> [Might be worth posting the rebase separately in the future, so that it's easy
> to find the material change.]

This patch is quite different than the last one that has been reviewed so I need
a new review. Also I thought nowadays the review extension makes it unnecessary
to upload the rebase, isn't it the case?

[kbalazs, 2014-03-13 23:06:58]

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:386: SetUpInDeathAssert();
On 2014/03/13 22:30:33, shess wrote:
> This shouldn't be a death test, right?

No I just tried to reuse the class because I need to do the same initialization.
I will improve.

[Scott Hess - ex-Googler, 2014-03-13 23:18:42]

OK, since you said it needed a new overall review, I tried to give it one. 
Which means I've probably scratched open old wounds.  But I think my suggestions
are minor enough that we can circle around again before the entire codebase is
rewritten :-).

Thanks for sticking to this.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:511: return *result =
g_old_malloc(malloc_default_zone(), size);
No return here to be consistent with else case.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:516: return *result;
!= NULL to be consistent with calloc (and other platforms).

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:535: bool result ALLOW_UNUSED = UncheckedMalloc(size,
&address);
Is |address| guaranteed to be initialized to NULL?  The implementation of
two-parameter UncheckedMalloc() guarantees that address will be NULL'ed out, but
this might be cleaner as the obvious:
  return UncheckedMalloc(size, &address) ? address : NULL;
or the if() of your choosing.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:386: SetUpInDeathAssert();
On 2014/03/13 23:07:00, kbalazs wrote:
> On 2014/03/13 22:30:33, shess wrote:
> > This shouldn't be a death test, right?
> 
> No I just tried to reuse the class because I need to do the same
initialization.
> I will improve.

Yeah, I think "death test" has meaning that you don't want to invoke.

I'm not sure how purist it's worth being on this.  You could use TEST(X, Y), but
X can't be the same as the X from a TEST_F().  Clearly value_ can just be
converted to a local, and I bet that this case doesn't need the other setup. 
But then you need a way to get access to the test_size_.

[kbalazs, 2014-03-18 20:03:53]

PTAL

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac.mm
File base/process/memory_mac.mm (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:511: return *result =
g_old_malloc(malloc_default_zone(), size);
On 2014/03/13 23:18:45, shess wrote:
> No return here to be consistent with else case.

Done.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:516: return *result;
On 2014/03/13 23:18:45, shess wrote:
> != NULL to be consistent with calloc (and other platforms).

Done.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_mac...
base/process/memory_mac.mm:535: bool result ALLOW_UNUSED = UncheckedMalloc(size,
&address);
On 2014/03/13 23:18:45, shess wrote:
> Is |address| guaranteed to be initialized to NULL?  The implementation of
> two-parameter UncheckedMalloc() guarantees that address will be NULL'ed out,
but
> this might be cleaner as the obvious:
>   return UncheckedMalloc(size, &address) ? address : NULL;
> or the if() of your choosing.

Done.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:386: SetUpInDeathAssert();
On 2014/03/13 23:18:45, shess wrote:
> On 2014/03/13 23:07:00, kbalazs wrote:
> > On 2014/03/13 22:30:33, shess wrote:
> > > This shouldn't be a death test, right?
> > 
> > No I just tried to reuse the class because I need to do the same
> initialization.
> > I will improve.
> 
> Yeah, I think "death test" has meaning that you don't want to invoke.
> 
> I'm not sure how purist it's worth being on this.  You could use TEST(X, Y),
but
> X can't be the same as the X from a TEST_F().  Clearly value_ can just be
> converted to a local, and I bet that this case doesn't need the other setup. 
> But then you need a way to get access to the test_size_.

I factored the common code into a base class.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:388: EXPECT_TRUE(value_ != NULL);
On 2014/03/13 22:30:33, shess wrote:
> When these succeed, you'll need to free value_.

Done.

https://codereview.chromium.org/55333002/diff/3240001/base/process/memory_uni...
base/process/memory_unittest.cc:399: EXPECT_EQ(memcmp(value_, zero, 512), 0);
On 2014/03/13 22:30:33, shess wrote:
> I think prefer a kConstant to copies of 512.
> 
> I think the obvious for() loop would be clearer than crafting a buffer to
> memcmp().

Done.

[Scott Hess - ex-Googler, 2014-03-18 21:10:37]

LGTM.

IMHO, you should fix my nits and not do another review cycle.  I'd hate to lose
this CL due to fatigue.  And history indicates we'll all get another chance to
review it in a few hours anyhow :-).

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
File base/process/memory_unittest.cc (right):

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
base/process/memory_unittest.cc:168: #if defined(USE_TCMALLOC)
Indentation.

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
base/process/memory_unittest.cc:186: void SetUpInDeathAssert() {
I think the function is indented 2 spaces too much.

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
base/process/memory_unittest.cc:397: base::EnableTerminationOnOutOfMemory();
Suggest a comment on this line about being important to the tests.  If it is
removed, the tests will probably still succeed, and I cannot think of any way to
productively test that the setup is correct.

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
base/process/memory_unittest.cc:415: EXPECT_EQ(bytes[i], 0);
EXPECT_EQ(expected, actual), so 0 on the left, bytes[i] on the right.

https://codereview.chromium.org/55333002/diff/3260001/base/process/memory_uni...
base/process/memory_unittest.cc:423: EXPECT_EQ(bytes[i], 0);
Also here.

[kbalazs, 2014-03-21 05:39:44]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-03-21 05:40:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001

[commit-bot: I haz the power, 2014-03-21 05:41:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 05:41:59]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[kbalazs, 2014-03-21 16:00:31]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-03-21 16:01:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001

[commit-bot: I haz the power, 2014-03-21 17:42:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-21 17:42:30]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[kbalazs, 2014-03-21 19:58:40]

The CQ bit was checked by b.kelemen@samsung.com

[commit-bot: I haz the power, 2014-03-21 19:58:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/b.kelemen@samsung.com/55333002/3290001

[commit-bot: I haz the power, 2014-03-21 22:07:07]

Change committed as 258681

[kbalazs, 2014-03-21 23:40:34]

On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote:
> Change committed as 258681

I can't believe. It has landed!

[Scott Hess - ex-Googler, 2014-03-21 23:42:03]

On 2014/03/21 23:40:34, kbalazs wrote:
> On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote:
> > Change committed as 258681
> 
> I can't believe. It has landed!

Comment #161.  +1

[earthdok, 2014-03-26 15:37:13]

UncheckedCalloc() and UncheckedMalloc() would crash on OOM under
AddressSanitizer, because ASan's allocator crashes on OOM instead of returning
NULL. If your unit tests don't crash under ASan, it's only because you put the
tests under "#if !defined(ADDRESS_SANITIZER)".

We could make ASan return NULL on OOM by passing allocator_may_return_null=1,
but that would make _all_ OOMs non-fatal in ASan builds, not just the unchecked
ones. We can't afford to lose coverage like that.

We could also introduce a special interface for unchecked allocations in ASan.
But perhaps there is a way to work around this on the Chromium side?

[kbalazs, 2014-03-26 17:11:22]

On 2014/03/26 15:37:13, earthdok wrote:
> UncheckedCalloc() and UncheckedMalloc() would crash on OOM under
> AddressSanitizer, because ASan's allocator crashes on OOM instead of returning
> NULL. If your unit tests don't crash under ASan, it's only because you put the
> tests under "#if !defined(ADDRESS_SANITIZER)".
> 
> We could make ASan return NULL on OOM by passing allocator_may_return_null=1,
> but that would make _all_ OOMs non-fatal in ASan builds, not just the
unchecked
> ones. We can't afford to lose coverage like that.
> 
> We could also introduce a special interface for unchecked allocations in ASan.
> But perhaps there is a way to work around this on the Chromium side?

I thought it's ok to disable my tests on asan because all oom tests were already
disabled.
I thought this comment is right: 

// AddressSanitizer and ThreadSanitizer define the malloc()/free()/etc.
// functions so that they don't crash if the program is out of memory, so the
// OOM tests aren't supposed to work.

If that would be the case than I don't think we should do anything. If the oom
killer doesn't work on asan than there is nothing to test here.

But it seems like you disagree with that, right? So, do you think we should make
all the oom tests work on asan? For UncheckedMalloc it requires special
interface indeed. For the other tests it should just work if oom is fatal on
asan.

[Scott Hess - ex-Googler, 2014-03-26 17:19:51]

On 2014/03/26 17:11:22, kbalazs wrote:
> On 2014/03/26 15:37:13, earthdok wrote:
> > UncheckedCalloc() and UncheckedMalloc() would crash on OOM under
> > AddressSanitizer, because ASan's allocator crashes on OOM instead of
returning
> > NULL. If your unit tests don't crash under ASan, it's only because you put
the
> > tests under "#if !defined(ADDRESS_SANITIZER)".
> > 
> > We could make ASan return NULL on OOM by passing
allocator_may_return_null=1,
> > but that would make _all_ OOMs non-fatal in ASan builds, not just the
> unchecked
> > ones. We can't afford to lose coverage like that.
> > 
> > We could also introduce a special interface for unchecked allocations in
ASan.
> > But perhaps there is a way to work around this on the Chromium side?
> 
> I thought it's ok to disable my tests on asan because all oom tests were
already
> disabled.
> I thought this comment is right: 
> 
> // AddressSanitizer and ThreadSanitizer define the malloc()/free()/etc.
> // functions so that they don't crash if the program is out of memory, so the
> // OOM tests aren't supposed to work.
> 
> If that would be the case than I don't think we should do anything. If the oom
> killer doesn't work on asan than there is nothing to test here.
> 
> But it seems like you disagree with that, right? So, do you think we should
make
> all the oom tests work on asan? For UncheckedMalloc it requires special
> interface indeed. For the other tests it should just work if oom is fatal on
> asan.

We periodically ship ASAN-built Chromium to a canary, so this isn't just a
testing question.

Honestly, though, the cases where handling NULL return apply are pretty narrow
and contentious.  By volume I'm sure most of the NULL returns come a few
seconds/minutes before your renderer crashes due to some other OOM case.  So I'm
not sure that it would be worth the development effort to really fix this,
unless it turns out to be really easy to do.

[earthdok, 2014-03-28 13:24:50]

Sorry for the late reply - forgot to CC myself on this review.

On 2014/03/26 17:11:22, kbalazs wrote:
> I thought it's ok to disable my tests on asan because all oom tests were
already
> disabled.
> I thought this comment is right: 
> 
> // AddressSanitizer and ThreadSanitizer define the malloc()/free()/etc.
> // functions so that they don't crash if the program is out of memory, so the
> // OOM tests aren't supposed to work.

That comment is both outdated and misleading. AddressSanitizer sanity-checks the
requested allocation size, and it used to return NULL if the size was
unreasonably large (> 2^36), which is the case for the OOM tests. This is no
longer the default behavior. We now crash by default (unless
allocator_may_return_null is set). Thus we can now enable the OOM tests in ASan
builds (I'll submit a patch later).

Apart from that we have the case of a genuine OOM when trying to allocate a
reasonable-sized chunk. This unconditionally causes a crash in ASan, and this
has always been the case (this is where my own reply was also misleading).

On 2014/03/26 17:19:51, shess wrote:
> Honestly, though, the cases where handling NULL return apply are pretty narrow
> and contentious.  By volume I'm sure most of the NULL returns come a few
> seconds/minutes before your renderer crashes due to some other OOM case.  So
I'm
> not sure that it would be worth the development effort to really fix this,
> unless it turns out to be really easy to do.

In that case I'm leaning towards simply adding a warning above UncheckedMalloc()
to the effect that that it's not guaranteed to not crash. However, I would like
to have a better understanding of why we need this function at all. Currently
Skia seems to be the only user, can anyone please describe the rationale for the
use of unchecked allocs in Skia?

[Scott Hess - ex-Googler, 2014-03-28 14:09:18]

On 2014/03/28 13:24:50, earthdok wrote:
> However, I would like
> to have a better understanding of why we need this function at all. Currently
> Skia seems to be the only user, can anyone please describe the rationale for
the
> use of unchecked allocs in Skia?

The goal is not to crash the renderer if a web page contains a corrupt image
with huge dimensions, or a huge canvas, or a typed array or ArrayBuffer.  Might
be other cases.

http://crbug.com/73751 has some color.  Also http://crbug.com/117949 .

Personally, my bias has been that if a web page sad-tabs due to an OOM of this
sort, it's unlikely to be both a real working web page AND not sad tab in a few
seconds or minutes from a different sort of OOM.  But I implemented
UncheckedMalloc() for OSX because I was tired of hearing about it for years and
I had just done a deep dive into the OSX malloc library and had thought of how
to implement it.  It is broken again for OSX 10.9, and I'm still trying to think
of a reasonable way to get it working again.

[alkondratenko, 2014-04-02 05:12:36]

On 2014/03/21 22:07:07, I haz the power (commit-bot) wrote:
> Change committed as 258681

Corresponding upstream commit can be seen here:
https://github.com/alk/gperftools/commit/0399af1019240e2d9127a588ddc8e31ff465...

Feel free to review it as well.