Closed
Bug 1007549
Opened 9 years ago
Closed 9 years ago
Clean up allocateFromArenaInline()
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: n.nethercote, Assigned: n.nethercote)
Details
Attachments
(1 file, 1 obsolete file)
6.04 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
allocateFromArenaInline() can be made simpler.
![]() |
Assignee | |
Comment 1•9 years ago
|
||
Here's the current structure: if BF is happening chunk = PickChunk if (!chunk) if there's a partly-filled arena allocate from that and return chunk = PickChunk() allocate from chunk and return Here's the new structure: if BF isn't happening if there's a partly-filled arena allocate from that and return chunk = PickChunk() allocate from chunk and return I might be able to simplify things more -- I don't know if the GC lock is needed in order to set *bfs. If it's not, then I can move the GC lock creation down to just above PickChunk(). Whether *bfs needs locking appears to be tricky question judging from bug 844759. My rule of thumb w.r.t. |volatile| is that it doesn't do anything -- gwagner said he knows that's the case at least sometimes on ARM -- so I don't understand how the current code can be safe.
Attachment #8419230 -
Flags: review?(jcoppeard)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
This comment is incorrect because it doesn't matter where in the list we put the new arena -- we'll always allocate from it because we set freeLists[] to point to it.
Attachment #8419232 -
Flags: review?(jcoppeard)
Updated•9 years ago
|
Attachment #8419230 -
Flags: review?(jcoppeard) → review+
Comment 3•9 years ago
|
||
Comment on attachment 8419232 [details] [diff] [review] (part 2) - Remove an incorrect comment in allocateFromArenaInline() Review of attachment 8419232 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ -1530,5 @@ > * it to the list as a fully allocated arena. > - * > - * We add the arena before the the head, so that after the GC the most > - * recently added arena will be used first for allocations. This improves > - * cache locality. I think this is talking about what happens after a subsequent GC, not immediately. Adding new arenas at the front here means that they will end up at the front of the list section of non-full arenas after a GC, should they happen not to be full at that point.
Attachment #8419232 -
Flags: review?(jcoppeard)
Comment 4•9 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1) > Whether *bfs needs locking appears to be tricky question judging from bug > 844759. My rule of thumb w.r.t. |volatile| is that it doesn't do anything -- > gwagner said he knows that's the case at least sometimes on ARM -- so I don't > understand how the current code can be safe. I had forgotten about that bug. I'll try and land the patch to make it use atomics rather than volatile anyway, even if it doesn't fix all the TSAN issues. I think the use of volatile here is ok though. *bfs is set to BFS_RUN from the main thread, so if volatile doesn't do anything the worst that can happen is that we won't see the state change to BFS_JUST_FINISHED or BFS_DONE and we will take the lock. Then when we re-check *bfs we will see the change. However thread safety is always tricky and there's probably some details I haven't thought of so better to be safe than sorry.
![]() |
Assignee | |
Comment 5•9 years ago
|
||
> I had forgotten about that bug. I'll try and land the patch to make it use
> atomics rather than volatile anyway, even if it doesn't fix all the TSAN
> issues.
Ok. Maybe you could move |maybeLock| further down when you do that (and change it to an always-lock instead of a maybe-lock).
![]() |
Assignee | |
Comment 6•9 years ago
|
||
Comment on attachment 8419232 [details] [diff] [review] (part 2) - Remove an incorrect comment in allocateFromArenaInline() Review of attachment 8419232 [details] [diff] [review]: ----------------------------------------------------------------- BTW, don't be afraid to r- for a patch that's totally wrong. I think that's better than cancelling review because it's clearer that (a) you've reviewed it, and (b) you don't like it. As for "you're heading in the right direction but you need to fix some things and I want to see it again" reviews, I use f+ rather than cancelling the review.
Attachment #8419232 -
Flags: review-
![]() |
Assignee | |
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a9a7e73eb1
![]() |
Assignee | |
Updated•9 years ago
|
Attachment #8419232 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/93a9a7e73eb1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•