Page MenuHomePhabricator

Added 'inline' attribute to basic_string's destructor
ClosedPublic

Authored by laxmansole on Jul 26 2016, 3:30 PM.

Details

Summary

Currently basic_string's destructor is not getting inlined. So adding 'inline' attribute to ~basic_string().

Worked in collaboration with Aditya Kumar.

Diff Detail

Event Timeline

laxmansole updated this revision to Diff 65591.Jul 26 2016, 3:30 PM
laxmansole retitled this revision from to Added 'inline' attribute to basic_string's destructor.
laxmansole updated this object.
laxmansole added subscribers: cfe-commits, sebpop, hiraditya and 2 others.
EricWF added a subscriber: EricWF.Aug 2 2016, 10:48 PM

LGTM.

However I would like to see a small benchmark that demonstrates the performance change. Please try and write the benchmark using Google Benchmark.
Some helpful links:

http://libcxx.llvm.org/docs/TestingLibcxx.html#building-benchmarks
http://github.com/google/benchmark

Let me know if I can help.

LGTM.

However I would like to see a small benchmark that demonstrates the performance change. Please try and write the benchmark using Google Benchmark.
Some helpful links:

http://libcxx.llvm.org/docs/TestingLibcxx.html#building-benchmarks
http://github.com/google/benchmark

Let me know if I can help.

Sure,
We'll come up with a synthetic benchmark to expose performance improvements.

Thanks,

LGTM.

However I would like to see a small benchmark that demonstrates the performance change. Please try and write the benchmark using Google Benchmark.
Some helpful links:

http://libcxx.llvm.org/docs/TestingLibcxx.html#building-benchmarks
http://github.com/google/benchmark

Let me know if I can help.

Hi Eric,
Is this also good to merge.

Thanks,
-Aditya

mclow.lists edited edge metadata.EditedAug 22 2016, 11:58 AM

@EricWF - did you want to see a benchmark as part of this commit?

No benchmark needed but one would be appreciated if possible.

No benchmark needed but one would be appreciated if possible.

I guess @laxmansole sent you a synthetic benchmark because we were having trouble integrating it to the libcxx testsuite. I can send you that again which can be submitted as a separate patch if you want.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 7 2019, 6:47 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:47 AM