This is an archive of the discontinued LLVM Phabricator instance.

[demangler] Improve buffer hysteresis
ClosedPublic

Authored by urnathan on Feb 16 2022, 1:22 PM.

Details

Summary

In fixing the std:max problem with the earlier patch, I noticed we could do better here by adjusting how the allocator hysteresis is used.

Except in the initialization case, if we needed more than double the buffer, the original code would allocate exactly the amount needed, and thus consequently the next request would also realloc. Just add the hysteresis to what we need everytime we know we're going to allocate. As the first request is probably a few bytes, avoid allocating just over 1K by dialing the hysteresis back a bit.

Diff Detail

Event Timeline

urnathan requested review of this revision.Feb 16 2022, 1:22 PM
urnathan created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2022, 1:22 PM

I found it is possible to add a test for this in llvm/unittest/Demangle/. The unittest uses google test and we could test the number of reallocation in a certain input. I think it is always better to add a test when we make a non-NFC change.

urnathan updated this revision to Diff 409591.Feb 17 2022, 4:37 AM

Added unit test

Oh, the test looks not straight forward. I am confused at the first sight. And I figure it out after thinking for a while. I could understand it since I have the revision background. I guess other people reading the test might be confused. I have two suggestion:

  1. Add a debug counter wrapped by #ifndef NDEBUG and a corresponding test wrapped by #ifndef NDEBUG. This is friendly for reader to read.
  2. Or at least create another test with new name Reallocation and a corresponding comment.

Oh, the test looks not straight forward. I am confused at the first sight. And I figure it out after thinking for a while. I could understand it since I have the revision background. I guess other people reading the test might be confused. I have two suggestion:

  1. Add a debug counter wrapped by #ifndef NDEBUG and a corresponding test wrapped by #ifndef NDEBUG. This is friendly for reader to read.
  2. Or at least create another test with new name Reallocation and a corresponding comment.

I think you're making this far too complicated.

Oh, the test looks not straight forward. I am confused at the first sight. And I figure it out after thinking for a while. I could understand it since I have the revision background. I guess other people reading the test might be confused. I have two suggestion:

  1. Add a debug counter wrapped by #ifndef NDEBUG and a corresponding test wrapped by #ifndef NDEBUG. This is friendly for reader to read.
  2. Or at least create another test with new name Reallocation and a corresponding comment.

I think you're making this far too complicated.

I don't think so. I think the reader who reads the test would be confused really. And it wouldn't be complicated. All we need to do is to add a new TEST and a more detailed comment. I think it wouldn't take too many lines.

It would be great to backup this kind of fine-tuning with some benchmarks, otherwise we're just making the code more complex without data :-/

urnathan updated this revision to Diff 411857.Feb 28 2022, 11:37 AM

separate unit test

It would be great to backup this kind of fine-tuning with some benchmarks, otherwise we're just making the code more complex without data :-/

We're actually making it simpler. Replacing a conditional assignment with an unconditional add.

ChuanqiXu accepted this revision.Feb 28 2022, 5:43 PM

Benchmarking is always good. But the cost may be too high sometimes.

llvm/unittests/Demangle/OutputBufferTest.cpp
89

Thanks. I feel better if there is a comment.

This revision is now accepted and ready to land.Feb 28 2022, 5:43 PM
This revision was landed with ongoing or failed builds.Mar 1 2022, 4:38 AM
This revision was automatically updated to reflect the committed changes.
urnathan marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 4:38 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript