Skip to content
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.

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

safer delete #227

Merged
merged 1 commit into from
Jul 3, 2019
Merged

safer delete #227

merged 1 commit into from
Jul 3, 2019

Conversation

slava77
Copy link
Collaborator

@slava77 slava77 commented Jun 3, 2019

delete [] x -> operator delete [] (x)
to avoid locks in jemalloc.

This came up in tests with CMSSW.

@kmcdermo
Copy link
Collaborator

kmcdermo commented Jun 3, 2019

@slava77 , can you post the benchmarks? this seems like a fix that should go in right away

@slava77
Copy link
Collaborator Author

slava77 commented Jun 3, 2019

@slava77 , can you post the benchmarks? this seems like a fix that should go in right away

I'm not setup to run benchmarks atm

@srlantz srlantz mentioned this pull request Jun 4, 2019
@makortel
Copy link
Collaborator

makortel commented Jul 3, 2019

Could we get this one integrated soon? :)

@srlantz
Copy link
Collaborator

srlantz commented Jul 3, 2019

Standard operating procedure is to run the benchmarks and verify there are no bad side effects before the pull request is merged. That's the only thing standing in the way of this PR as far as I know.

@srlantz srlantz requested a review from osschar July 3, 2019 18:02
@tresreid
Copy link
Collaborator

tresreid commented Jul 3, 2019

I have run benchmarks for this. They can be found here: http://mireid.web.cern.ch/mireid/Benchmarks/benchmarks_7_3_19_saferdel/
However, I think Slava's repo might have been private since i was not able to access it. I ended up just making the changes by hand instead of pulling this branch. Seeing as though this was only two lines, i don't think that should be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants