Task Tracer: Deleting TLS doesn't function properly

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: shelly, Assigned: cyu)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
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

4 years ago
Blocks: 995058
(Reporter)

Updated

4 years ago
Summary: Task Tracer: Deleting TSL doesn't function properly → Task Tracer: Deleting TLS doesn't function properly
(Reporter)

Updated

4 years ago
Depends on: 992454
(Reporter)

Updated

4 years ago
No longer blocks: 995058
(Reporter)

Updated

4 years ago
Blocks: 995058
Component: Gecko Profiler → MFBT
(Assignee)

Comment 1

4 years ago
Created attachment 8504610 [details] [diff] [review]
Part 1: Add cleanup function to ThreadLocal
Attachment #8504610 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

4 years ago
Created attachment 8504613 [details] [diff] [review]
Part 2: Test case
Assignee: nobody → cyu
Attachment #8504613 - Flags: review?(jwalden+bmo)

Comment 4

4 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 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

4 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

4 years ago
Attachment #8504613 - Flags: review?(jwalden+bmo) → review-

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.