508037 - expose new and FixedMalloc zeroing options (opt in)

Status: VERIFIED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect)

Description

[Tommy Reilly]

Attachment: zero memory for naked new calls

As part of OOM shutdown uninitialized variables can cause crashes during shutdown, new'ing calls to new prevents this.   Perf evaluated in shell and player and is acceptable, plain new just isn't used that much to allocate memory, most high traffic memory sites use specialized FixedAlloc instances already to avoid locking overhead.

Comment 1

[Edwin Smith]

Comment on attachment 392254 [details] [diff] [review]
zero memory for naked new calls

We have work in progress to convert several areas of GC allocations to fixed allocations, which requires initializing members in the constructor.  having the allocator zero memory will hide initialization bugs and is sometimes a measureable slowdown.  We've seen speedups in nanojit from *not* zero-initializing various structs.  

This seems like a very heavy-handed fix the shutdown crashes.  shouldn't the class explicitly initialize pointer members to NULL, to ensure they aren't accidentally freed at shutdown?

Comment 2

[Tommy Reilly]

This is mostly for our clients benifit where they have hundreds of classes and fixing all the constructors isn't feasible.  Antti's team just finished doing a perf evaluation on winmo and it showed no slow down.  Our experience with pre-zero'd memory was positive for GC memory and if we take advantage of this by removing init to zero code we should have smaller faster software.  As in the player I would suspect with NJ, super frequent allocations will go through a specialized allocator and not use new.  We're committed to making player as crash proof as possible and since we don't have time to comb through every constructor to make sure it zero's all pointers before doing any allocations (kinda of a weird constraint to put on ctors anyway) we were planning on relying on this patch to achieve a relatively crash proof solution.

Comment 3

[Tommy Reilly]

Attachment: adds flag, no zero'ing by default

Same as last patch only don't zero by default

Comment 4

[Tommy Reilly]

Attachment: remove bogus comment

same at last w/o incorrect comment

Comment 5

[Edwin Smith]

Comment on attachment 392710 [details] [diff] [review]
remove bogus comment

did global new zero by default before this change?

by opt-in, i thought that global-new would not zero, and if you wanted zeroing then you'd use an explicit api of some kind.  I dont think we want the default global new to zero, especially as we're about to change the global new to be system-new (optionally), which also doesn't zero.

Comment 6

[Edwin Smith]

to expand, i think we want this general policy: anyone calling system-new, default-new, or VMPI_alloc, or malloc(), should be taking care to explicitly initialize things.  

anyone calling a custom overloaded new with a please-zero parameter (or implied by the api), can rely on zeroing

when macros come into the picture, i think this means:

mmfx_new -> doesn't zero
system_new -> doesn't zero

mmgc_new -> zero's

Comment 7

[Tommy Reilly]

This is exactly that this change does, it justs adds a parameter to FixedAlloc, new capability no changes.

Comment 8

[Edwin Smith]

Comment on attachment 392710 [details] [diff] [review]
remove bogus comment

foold by last patch hunk... GCRoot::new zeros by default, but not global new.

Comment 9

[Tommy Reilly]

changeset 2360