Page MenuHomePhabricator

Make the static counters in ASTContext non-static
ClosedPublic

Authored by alexfh on Feb 25 2019, 5:25 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

alexfh created this revision.Feb 25 2019, 5:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 5:25 AM
Herald added a subscriber: jfb. · View Herald Transcript

Hmm. These are not the only static variables which are used for statistics (eg: in DeclBase.cpp). Would it make sense instead to keep all of the statistics in the AST context (without making them atomic) ?

Hmm. These are not the only static variables which are used for statistics (eg: in DeclBase.cpp). Would it make sense instead to keep all of the statistics in the AST context (without making them atomic) ?

+1, the only reason to use atomics here instead is if getting hold of an actual ASTContext is hard where the statistics are used (for legacy reasons?).

Hmm. These are not the only static variables which are used for statistics (eg: in DeclBase.cpp). Would it make sense instead to keep all of the statistics in the AST context (without making them atomic) ?

+1, the only reason to use atomics here instead is if getting hold of an actual ASTContext is hard where the statistics are used (for legacy reasons?).

I don't know how hard it would be to do this, but I would like to argue that this should be done even if it require some refactoring. These static variables used for stats are kind of ugly imho; they conceptually belong to the AST context.

I don't know how hard it would be to do this, but I would like to argue that this should be done even if it require some refactoring. These static variables used for stats are kind of ugly imho; they conceptually belong to the AST context.

I agree, OTOH if refactoring would take more time than one can spare, moving them to atomics is better than what we have now.
For example, AFAIK that's the only thing that keeps clang from being tsan-clean.

riccibruno added a comment.EditedFeb 25 2019, 6:06 AM

I don't know how hard it would be to do this, but I would like to argue that this should be done even if it require some refactoring. These static variables used for stats are kind of ugly imho; they conceptually belong to the AST context.

I agree, OTOH if refactoring would take more time than one can spare, moving them to atomics is better than what we have now.
For example, AFAIK that's the only thing that keeps clang from being tsan-clean.

Fair enough. Is that urgent ? I actually would be willing to look at this since I wanted to add some stats to track the space used for trailing objects anyway.

alexfh updated this revision to Diff 188163.Feb 25 2019, 6:52 AM

Make the counters non-static members of ASTContext.

I don't know how hard it would be to do this, but I would like to argue that this should be done even if it require some refactoring. These static variables used for stats are kind of ugly imho; they conceptually belong to the AST context.

I agree, OTOH if refactoring would take more time than one can spare, moving them to atomics is better than what we have now.
For example, AFAIK that's the only thing that keeps clang from being tsan-clean.

Fair enough. Is that urgent ? I actually would be willing to look at this since I wanted to add some stats to track the space used for trailing objects anyway.

It turned out to be a trivial change. Thanks for the idea, btw ;)

Okay, but what about the other similar uses of static members which have the same problem ?

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

which count the number of declaration node of each kind.

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

which count the number of declaration node of each kind.

Okay, but what about the other similar uses of static members which have the same problem ?

Do you have any example in mind? I've only seen TSan warnings for these counters, nothing else so far.

For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

which count the number of declaration node of each kind.

These are probably easier to convert to std::atomic<int>, but I'd do this separately.

riccibruno accepted this revision.Feb 25 2019, 7:57 AM

Sounds good. LGTM.

This revision is now accepted and ready to land.Feb 25 2019, 7:57 AM

Awesome, I was also surprised it's so easy to convert them.

(With a commit message which actually reflect the change of course)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 8:11 AM

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

These are probably easier to convert to std::atomic<int>, but I'd do this separately.

Just converting these to std::atomic<> will mute TSan, but won't fix the underlying issue: the stats should be collected per-compiler instance. Initialization will require a bit more locking as well. Not sure what would be a good solution here.

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

These are probably easier to convert to std::atomic<int>, but I'd do this separately.

Just converting these to std::atomic<> will mute TSan, but won't fix the underlying issue: the stats should be collected per-compiler instance. Initialization will require a bit more locking as well. Not sure what would be a good solution here.

I agree that making them atomic is not a good solution. I am not sure that the stats should be stored in the compiler instance (is that what you are suggesting ?). The number of declaration nodes of each kind seems to be something that should be stored in the AST context.

riccibruno retitled this revision from Use std::atomic<> for static counters. to Make the static counters in ASTContext non-static.Feb 25 2019, 9:02 AM
riccibruno edited the summary of this revision. (Show Details)
riccibruno changed the repository for this revision from rL LLVM to rC Clang.
riccibruno removed a project: Restricted Project.

...
For example in DeclBase.cpp

#define DECL(DERIVED, BASE) static int n##DERIVED##s = 0;
#define ABSTRACT_DECL(DECL)
#include "clang/AST/DeclNodes.inc"

...

These are probably easier to convert to std::atomic<int>, but I'd do this separately.

Just converting these to std::atomic<> will mute TSan, but won't fix the underlying issue: the stats should be collected per-compiler instance. Initialization will require a bit more locking as well. Not sure what would be a good solution here.

I agree that making them atomic is not a good solution. I am not sure that the stats should be stored in the compiler instance (is that what you are suggesting ?). The number of declaration nodes of each kind seems to be something that should be stored in the AST context.

Well, I wasn't talking about storing the metrics in the CompilerInstance class directly. Maybe in ASTContext. But after looking a bit closer I see that the problem is that an instance of ASTContext is not readily available where these stats are gathered. I'm not sure I'll get to change this any time soon though.