1423000 - Miscellaneous changes, mostly logalloc-related

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement)

Comment 1

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Remove Replay::Commit.

It adds an uncompressible and noticeable time overhead to replaying
logs, even when one is not interested in measuring RSS. This has caused
me to clear the method body on multiple occasions.

If necessary, it is possible to enable zero or junk at the allocator
level for the same effect.

Review commit: https://reviewboard.mozilla.org/r/205256/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205256/

Comment 2

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Count jemalloc_stats as an operation.

While jemalloc_stats is not actively doing anything, it can be
cumbersome to not have it count as an operation, because the operation
count shown on jemalloc_stats doesn't match the line number in the input
replay log, and the offset grows as the number of jemalloc_stats
operations grows.

While here, also update a comment about the replay log format.

Review commit: https://reviewboard.mozilla.org/r/205258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205258/

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Move MemSlot lookup for replay results into the Replay methods.

This allows all methods to have the same signature.

Review commit: https://reviewboard.mozilla.org/r/205260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205260/

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Reject some more invalid logalloc replay logs.

For functions with no result, such as free, it's invalid for some string
to appear after the closing parenthesis.

Review commit: https://reviewboard.mozilla.org/r/205262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205262/

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Move mozjemalloc mutexes to a separate header.

Also change the definition of the StaticMutex constructor to unconfuse clang-format.

Review commit: https://reviewboard.mozilla.org/r/205264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205264/

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Use mozjemalloc mutexes in logalloc.

Instead of the chromium one, which required some tricks.

Review commit: https://reviewboard.mozilla.org/r/205266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205266/

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Don't use a separate replace-malloc library for the minimal-logalloc test.

Instead, only register a minimal set of functions when an environment
variable is set.

Review commit: https://reviewboard.mozilla.org/r/205268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205268/

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Always log allocator function calls.

So far, logalloc has avoided logging calls that e.g. return null
pointers, but both to make the code more generic and to enable logging
of error conditions, we now log every call.

Review commit: https://reviewboard.mozilla.org/r/205270/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205270/

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Re-run clang-format on memory/build.

Most adjustements come from some recent .clang-format changes. A few
were overlooked from changes to the code.

Review commit: https://reviewboard.mozilla.org/r/205272/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205272/

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1423000 - Run clang-format in memory/replace/logalloc.

Review commit: https://reviewboard.mozilla.org/r/205274/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205274/

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8934328 [details]
Bug 1423000 - Remove Replay::Commit.

https://reviewboard.mozilla.org/r/205256/#review210846

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8934329 [details]
Bug 1423000 - Count jemalloc_stats as an operation.

https://reviewboard.mozilla.org/r/205258/#review210850

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8934330 [details]
Bug 1423000 - Move MemSlot lookup for replay results into the Replay methods.

https://reviewboard.mozilla.org/r/205260/#review210852

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8934331 [details]
Bug 1423000 - Reject some more invalid logalloc replay logs.

https://reviewboard.mozilla.org/r/205262/#review210854

Comment 15

[Nicholas Nethercote [inactive]]

Comment on attachment 8934332 [details]
Bug 1423000 - Move mozjemalloc mutexes to a separate header.

https://reviewboard.mozilla.org/r/205264/#review210856

Comment 16

[Nicholas Nethercote [inactive]]

Comment on attachment 8934333 [details]
Bug 1423000 - Use mozjemalloc mutexes in logalloc.

https://reviewboard.mozilla.org/r/205266/#review210858

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8934334 [details]
Bug 1423000 - Don't use a separate replace-malloc library for the minimal-logalloc test.

https://reviewboard.mozilla.org/r/205268/#review210860

Comment 18

[Nicholas Nethercote [inactive]]

Comment on attachment 8934335 [details]
Bug 1423000 - Always log allocator function calls.

https://reviewboard.mozilla.org/r/205270/#review210862

Consistency is good.

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8934336 [details]
Bug 1423000 - Re-run clang-format on memory/build.

https://reviewboard.mozilla.org/r/205272/#review210864

I guess this is fine, but I'm surprised clang-format is touching comments. I thought it didn't do that under the Mozilla configuration?

::: memory/build/mozjemalloc.cpp:1194
(Diff revision 1)
>  
> -// The arena associated with the current thread (per jemalloc_thread_local_arena)
> -// On OSX, __thread/thread_local circles back calling malloc to allocate storage
> -// on first access on each thread, which leads to an infinite loop, but
> -// pthread-based TLS somehow doesn't have this problem.
> +// The arena associated with the current thread (per
> +// jemalloc_thread_local_arena) On OSX, __thread/thread_local circles back
> +// calling malloc to allocate storage on first access on each thread, which
> +// leads to an infinite loop, but pthread-based TLS somehow doesn't have this
> +// problem.

I thought clang-format (in our configuration) didn't touch comments?

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8934337 [details]
Bug 1423000 - Run clang-format in memory/replace/logalloc.

https://reviewboard.mozilla.org/r/205274/#review210866

Comment 21

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #19)
> I thought clang-format (in our configuration) didn't touch comments?

bug 1414764 removed "ReflowComments: false" from .clang-format.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be9b555ba235
Remove Replay::Commit. r=njn
https://hg.mozilla.org/integration/autoland/rev/921cfea08410
Count jemalloc_stats as an operation. r=njn
https://hg.mozilla.org/integration/autoland/rev/7f16eb74e4d4
Move MemSlot lookup for replay results into the Replay methods. r=njn
https://hg.mozilla.org/integration/autoland/rev/4c6d9fbccc0f
Reject some more invalid logalloc replay logs. r=njn
https://hg.mozilla.org/integration/autoland/rev/2b43ef7e2f6d
Move mozjemalloc mutexes to a separate header. r=njn
https://hg.mozilla.org/integration/autoland/rev/62a286838ec6
Use mozjemalloc mutexes in logalloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/eecbbe09235b
Don't use a separate replace-malloc library for the minimal-logalloc test. r=njn
https://hg.mozilla.org/integration/autoland/rev/fb91909e795a
Always log allocator function calls. r=njn
https://hg.mozilla.org/integration/autoland/rev/d0de68944d46
Re-run clang-format on memory/build. r=njn
https://hg.mozilla.org/integration/autoland/rev/1225ddb1e845
Run clang-format in memory/replace/logalloc. r=njn

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/be9b555ba235
https://hg.mozilla.org/mozilla-central/rev/921cfea08410
https://hg.mozilla.org/mozilla-central/rev/7f16eb74e4d4
https://hg.mozilla.org/mozilla-central/rev/4c6d9fbccc0f
https://hg.mozilla.org/mozilla-central/rev/2b43ef7e2f6d
https://hg.mozilla.org/mozilla-central/rev/62a286838ec6
https://hg.mozilla.org/mozilla-central/rev/eecbbe09235b
https://hg.mozilla.org/mozilla-central/rev/fb91909e795a
https://hg.mozilla.org/mozilla-central/rev/d0de68944d46
https://hg.mozilla.org/mozilla-central/rev/1225ddb1e845