1007549 - Clean up allocateFromArenaInline()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [:njn] (On Long Service Leave)]

allocateFromArenaInline() can be made simpler.

Comment 1

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 1) - Simplify the control flow in allocateFromArenaInline()

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.

Comment 2

[Nicholas Nethercote [:njn] (On Long Service Leave)]

Attachment: (part 2) - Remove an incorrect comment in allocateFromArenaInline()

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.

Comment 3

[Jon Coppeard (:jonco)]

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.

Comment 4

[Jon Coppeard (:jonco)]

(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.

Comment 5

[Nicholas Nethercote [:njn] (On Long Service Leave)]

> 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).

Comment 6

[Nicholas Nethercote [:njn] (On Long Service Leave)]

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.

Comment 7

[Nicholas Nethercote [:njn] (On Long Service Leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/93a9a7e73eb1

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93a9a7e73eb1