[ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

Fangrui Song via cfe-dev
Cc John Thompson, but I don’t know his email address.

I recently ran across llvm::StringPool in llvm/lib/Support.  It is an old class which isn’t ideal in several ways, and I would like to remove it in favor of direct uses of llvm::StringSet, which is nearly the same thing and a lot more efficient.  The one difference is that in this one, each entry is individually reference counted, so they get deallocated as soon as the last reference is dropped.

The only use of this class is in PreprocessorTrackerImpl in clang-tools-extra/modularize/PreprocessorTracker.cpp.  Does anyone object to changing this to being a simple llvm::StringSet?  Such a change would make this more efficient (and allow deleting a misleading class out of llvm/lib/Support), allow simplifying away the "StringHandle” logic (in favor of using StringRef directly) and the only cost that I can see is that the strings won’t get deallocated as eagerly.

An alternative solution here is to move StringPool into clang-tools-extra as a private implementation detail.  I’d personally rather remove it outright than preserve it.

Any thoughts?

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

Fangrui Song via cfe-dev
I also noticed StringPool last month and had more or less the same thought: this looks dead, can we remove it?

+1 for replacing this usage with StringSet and removing StringPool.

On Mon, Apr 13, 2020 at 11:03 AM Chris Lattner via cfe-dev <[hidden email]> wrote:
Cc John Thompson, but I don’t know his email address.

I recently ran across llvm::StringPool in llvm/lib/Support.  It is an old class which isn’t ideal in several ways, and I would like to remove it in favor of direct uses of llvm::StringSet, which is nearly the same thing and a lot more efficient.  The one difference is that in this one, each entry is individually reference counted, so they get deallocated as soon as the last reference is dropped.

The only use of this class is in PreprocessorTrackerImpl in clang-tools-extra/modularize/PreprocessorTracker.cpp.  Does anyone object to changing this to being a simple llvm::StringSet?  Such a change would make this more efficient (and allow deleting a misleading class out of llvm/lib/Support), allow simplifying away the "StringHandle” logic (in favor of using StringRef directly) and the only cost that I can see is that the strings won’t get deallocated as eagerly.

An alternative solution here is to move StringPool into clang-tools-extra as a private implementation detail.  I’d personally rather remove it outright than preserve it.

Any thoughts?

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

Fangrui Song via cfe-dev
On 2020-04-14, Reid Kleckner via cfe-dev wrote:
>I also noticed StringPool last month and had more or less the same thought:
>this looks dead, can we remove it?
>
>+1 for replacing this usage with StringSet and removing StringPool.

+1. llvm/Support/StringSaver.h:UniqueStringSaver does more or less the
same thing.

>On Mon, Apr 13, 2020 at 11:03 AM Chris Lattner via cfe-dev <
>[hidden email]> wrote:
>
>> Cc John Thompson, but I don’t know his email address.
>>
>> I recently ran across llvm::StringPool in llvm/lib/Support.  It is an old
>> class which isn’t ideal in several ways, and I would like to remove it in
>> favor of direct uses of llvm::StringSet, which is nearly the same thing and
>> a lot more efficient.  The one difference is that in this one, each entry
>> is individually reference counted, so they get deallocated as soon as the
>> last reference is dropped.
>>
>> The only use of this class is in PreprocessorTrackerImpl in
>> clang-tools-extra/modularize/PreprocessorTracker.cpp.  Does anyone object
>> to changing this to being a simple llvm::StringSet?  Such a change would
>> make this more efficient (and allow deleting a misleading class out of
>> llvm/lib/Support), allow simplifying away the "StringHandle” logic (in
>> favor of using StringRef directly) and the only cost that I can see is that
>> the strings won’t get deallocated as eagerly.
>>
>> An alternative solution here is to move StringPool into clang-tools-extra
>> as a private implementation detail.  I’d personally rather remove it
>> outright than preserve it.
>>
>> Any thoughts?
>>
>> -Chris
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>

>_______________________________________________
>cfe-dev mailing list
>[hidden email]
>https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ClangToolsExtra] PreprocessorTrackerImpl's use of llvm::StringPool

Fangrui Song via cfe-dev
Cool, thanks!  I put up this patch to replace this usage in favor of StringSet - once this goes in, I’ll remove StringPool entirely.  I’d appreciate an LGTM, thanks!


-Chris

On Apr 15, 2020, at 11:17 AM, Fangrui Song <[hidden email]> wrote:

On 2020-04-14, Reid Kleckner via cfe-dev wrote:
I also noticed StringPool last month and had more or less the same thought:
this looks dead, can we remove it?

+1 for replacing this usage with StringSet and removing StringPool.

+1. llvm/Support/StringSaver.h:UniqueStringSaver does more or less the
same thing.

On Mon, Apr 13, 2020 at 11:03 AM Chris Lattner via cfe-dev <
[hidden email]> wrote:

Cc John Thompson, but I don’t know his email address.

I recently ran across llvm::StringPool in llvm/lib/Support.  It is an old
class which isn’t ideal in several ways, and I would like to remove it in
favor of direct uses of llvm::StringSet, which is nearly the same thing and
a lot more efficient.  The one difference is that in this one, each entry
is individually reference counted, so they get deallocated as soon as the
last reference is dropped.

The only use of this class is in PreprocessorTrackerImpl in
clang-tools-extra/modularize/PreprocessorTracker.cpp.  Does anyone object
to changing this to being a simple llvm::StringSet?  Such a change would
make this more efficient (and allow deleting a misleading class out of
llvm/lib/Support), allow simplifying away the "StringHandle” logic (in
favor of using StringRef directly) and the only cost that I can see is that
the strings won’t get deallocated as eagerly.

An alternative solution here is to move StringPool into clang-tools-extra
as a private implementation detail.  I’d personally rather remove it
outright than preserve it.

Any thoughts?

-Chris
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev