Several memory management improvements are desired: * used fixed allocation instead of GCAlloc for blockStates, FrameState * only the jit code path is protected by try/catch, all uses of the verifier should be. * verifier produces two un-freed FrameState's per invocation of verify() and one of them is entirely unused. Coder & LIR need cleanup as well, will use separate bugs.
Created attachment 384392 [details] [diff] [review] Convert verifier blockStates & FrameState & Value to fixed mem In this patch: * FrameState, blockStates, and Value allocated with FixedMem * Value array is a separate allocation from FrameState * TRY/CATCH around verifier handles interp-only invocation too * removed redundant initialState instance of FrameState * explicitly free verifier->state in addition to blockStates The only GC mem used by the verifier now is objects that get attached into the type system and live longer than the verifier: - ExceptionHandlerTable - ScopeTypeChain - things allocated by resolveTraits & resolveSignatures
Assignee: nobody → edwsmith
Attachment #384392 - Flags: review?(lhansen)
Comment on attachment 384392 [details] [diff] [review] Convert verifier blockStates & FrameState & Value to fixed mem Overall comment: new memory handling regime has lost some OOM checks. Are they done elsewhere? - scopeValue(i) could use an assertion to verify input as was done for value(i). scope overflow not very likely due to other checks? - The "// pass 2 - data flow" comment @ MethodInfo.cpp line 291 is obsolete? - Verifier::cleanup() could delete blockStates.at(i) -- delete NULL port issues? - Verifier::verify() passes self to FrameState ctor, but still has logic copying frame info into the new FrameState. Move into ctor? Unless NULL isn't binary zero on a platform, this loop looks redundant b/c the ctor does initialize the new FrameState's locals to 0.
Attachment #384392 - Flags: review?(tharwood) → review+
Note, out of memory errors are no longer possible unless specific APIs (FixedMalloc::PleaseAlloc) are used. NULL checks are entirely redundant.
Attachment #384392 - Flags: superreview?(lhansen) → superreview+
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.