Skip to content

Add dtor for CapturedValues to make its SListBase members to be exception safe #1395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chakrabot merged 1 commit into from
Aug 8, 2016
Merged

Add dtor for CapturedValues to make its SListBase members to be exception safe #1395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions lib/Backend/GlobOptBailOut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,10 @@ GlobOpt::CaptureValues(BasicBlock *block, BailOutInfo * bailOutInfo)
bailOutInfo->capturedValues.constantValues.Clear(this->func->m_alloc);
bailOutConstValuesIter.SetNext(&bailOutInfo->capturedValues.constantValues);
bailOutInfo->capturedValues.constantValues = capturedValues.constantValues;
capturedValues.constantValues.Reset();

bailOutInfo->capturedValues.copyPropSyms.Clear(this->func->m_alloc);
bailOutCopySymsIter.SetNext(&bailOutInfo->capturedValues.copyPropSyms);
bailOutInfo->capturedValues.copyPropSyms = capturedValues.copyPropSyms;
capturedValues.copyPropSyms.Reset();

if (!PHASE_OFF(Js::IncrementalBailoutPhase, func))
{
Expand Down
8 changes: 8 additions & 0 deletions lib/Backend/IR.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ struct CapturedValues
SListBase<ConstantStackSymValue> constantValues; // Captured constant values during glob opt
SListBase<CopyPropSyms> copyPropSyms; // Captured copy prop values during glob opt
BVSparse<JitArenaAllocator> * argObjSyms; // Captured arg object symbols during glob opt

~CapturedValues()
{
// Reset SListBase to be exception safe. Captured values are from GlobOpt->func->alloc
// in normal case the 2 SListBase are empty so no Clear needed, also no need to Clear in exception case
constantValues.Reset();
Copy link
Contributor

@curtisman curtisman Aug 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use clear instead. Especially if you are not using a local arena, but the Func arena, so the memorywill not be freed after Globopt. and be wasted until the Func finish JIT'ing.

Copy link
Collaborator Author

@ThomsonTan ThomsonTan Aug 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@curtisman , in normal cases, these 2 local SListBase would have 0 elements because it just host the values temporarily, all the values are moved to bailoutInfo at the end of value capture which will be cleared at BailOutInfo::Clear(). The only case is when exception happened and the nodes have not been moved to bailoutInfo. But in this case the function will abort compilation so no need to free nodes to func->alloc aggresively. This idea looks similar to SListBase, the code who uses it need to ensure memory are freed, not the SListBase itself (dtor).

Copy link
Contributor

@curtisman curtisman Aug 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

copyPropSyms.Reset();
}
};

class LoweredBasicBlock;
Expand Down