Closed
Bug 1064672
Opened 10 years ago
Closed 10 years ago
Task Tracer: Deleting TLS doesn't function properly
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: shelly, Assigned: cyu)
References
Details
Attachments
(2 files)
(deleted),
patch
|
Waldo
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review-
|
Details | Diff | Splinter Review |
TraceInfo of a content process needs to be flushed after itself is created by Nuwa. Otherwise the TraceInfo data of Nuwa process will carry over into this content process. We tend to flush the TraceInfo data by deleting and re-creating each TLS of TraceInfo, however, our <mozilla::ThreadLocal> does not implement destructor nor pthread_key_delete(), it causes memory leak eventually.
Reporter | ||
Updated•10 years ago
|
Summary: Task Tracer: Deleting TSL doesn't function properly → Task Tracer: Deleting TLS doesn't function properly
Component: Gecko Profiler → MFBT
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8504610 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → cyu
Attachment #8504613 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•10 years ago
|
||
Try push log: https://tbpl.mozilla.org/?tree=Try&rev=d901ecf098f0
Comment 4•10 years ago
|
||
Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal
Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------
Using thread-local storage and expecting automatic cleanup is probably not a good idea. It's almost always far better to have it be initialized and cleaned up by explicit operation.
Does this cause every ThreadLocal to require a static initializer? If it does, I think that's the nail in the coffin for this. If it doesn't, I'll have to ponder a little more.
Attachment #8504610 -
Flags: feedback?(nfroyd)
Comment 5•10 years ago
|
||
Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal
Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------
Yes, this will cause every (static, global-scope) ThreadLocal to require a static initializer. There are ways around this, of course:
ThreadLocal<T>& get() {
static ThreadLocal<T> instance;
return instance;
}
But I don't know that we want to go down that road. ThreadLocal requires explicit initialization right now, and encouraging patterns like the above runs counter to that. (Some people consider ThreadLocal's current behavior a bug, of course...)
Attachment #8504610 -
Flags: feedback?(nfroyd)
Comment 6•10 years ago
|
||
Comment on attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal
Review of attachment 8504610 [details] [diff] [review]:
-----------------------------------------------------------------
Yeah, I don't think we want this. ThreadLocal is supposed to be something roughly similar to C++11's thread_local storage duration. It is not a generic way to store something thread-locally, that's amenable to dynamic creation and destruction. And the static initializer issue only seals things, with the workaround not being something we are going to force on every user.
To sum it up, I don't think you should be using ThreadLocal for your use case. This would avoid leaking the TLS key, but it wouldn't avoid leaking any allocations the associated value keeps alive -- say, if someone had
static ThreadLocal<int*> intval;
intval.set(new int);
somewhere in their program, then didn't bother clearing it out and deleting the stored pointer prior to destruction.
I wouldn't be opposed to some sort of DynamicThreadLocal class in a separate header that did some of this. But don't try changing ThreadLocal for this purpose.
Attachment #8504610 -
Flags: review?(jwalden+bmo) → review-
Updated•10 years ago
|
Attachment #8504613 -
Flags: review?(jwalden+bmo) → review-
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•