BazelProfile: precompute some fields #195
Conversation
laszlocsomor
force-pushed
the
laszlo-precomp
branch
3 times, most recently
from
ab6e1c8 to
51837ad
Compare
| for (ProfileThread e : threads.values()) { | ||
| if (mainThread == null && isMainThread(e)) { | ||
| mainThread = e; | ||
| } else if (criticalPath == null && isCriticalPathThread(e)) { | ||
| criticalPath = e; | ||
| } else if (gcThread == null && isGarbageCollectorThread(e)) { | ||
| gcThread = e; | ||
| } | ||
| } |
There was a problem hiding this comment.
"e" for element. It makes no difference though.
There was a problem hiding this comment.
I'm assuming the question comes from e usually referring to an Exception?
There was a problem hiding this comment.
I can just use t.
| ImmutableMap.copyOf(otherData), | ||
| ImmutableMap.copyOf(threads), |
There was a problem hiding this comment.
For large profiles these copies are very costly (in terms of memory usage). How about we use Java's built-in Collections.unmodifiableMap instead to avoid copying the underlying data.
There was a problem hiding this comment.
Can you give more details, and what your concern is based on? How big do these maps get, what have you seen in the wild?
Sure, we can use unmodifiableMap. FWIW I could even use an ImmutableMap.Builder for otherData, but not for threads because we need to look up things while also building, and the ImmutableMap.Builder doesn't allow that.
There was a problem hiding this comment.
OH! That's my bad, I misread and thought this was (threadId) -> (List<Event>). It's not, sorry about that. I think ImmutableMap.copyOf is fine here.
|
ping |
iamricard
left a comment
iamricard
left a comment
There was a problem hiding this comment.
Sorry for the delay @laszlocsomor 🫠
| ImmutableMap.copyOf(otherData), | ||
| ImmutableMap.copyOf(threads), |
There was a problem hiding this comment.
OH! That's my bad, I misread and thought this was (threadId) -> (List<Event>). It's not, sorry about that. I think ImmutableMap.copyOf is fine here.
| for (ProfileThread e : threads.values()) { | ||
| if (mainThread == null && isMainThread(e)) { | ||
| mainThread = e; | ||
| } else if (criticalPath == null && isCriticalPathThread(e)) { | ||
| criticalPath = e; | ||
| } else if (gcThread == null && isGarbageCollectorThread(e)) { | ||
| gcThread = e; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm assuming the question comes from e usually referring to an Exception?
Some of the getters (getCriticalPath(), etc.) used to run in O(N) for N=threads.size(); now they are O(1) because we precompute what they'd return. Also, BazelProfile's members are no longer mutable, meaning we can safely precompute those values without fear of staleness. Signed-off-by: László Csomor <laszlo@engflow.com>
laszlocsomor
force-pushed
the
laszlo-precomp
branch
from
51837ad to
9ea3f60
Compare
laszlocsomor
left a comment
laszlocsomor
left a comment
There was a problem hiding this comment.
PTAL -- rebasing lead to significant changes
What's new:
otherDatais now calledotherDataBuilderand it's anImmutableMap.Builder- The Stream-based computation of
threads(frommainbranch) is gone; we compute the map withthreadsMapBuilderin the same for-loop as that looking for the special threads.
|
Thanks @iamricard ! |
Some of the getters (getCriticalPath(), etc.) used to run in O(N) for N=threads.size(); now they are O(1) because we precompute what they'd return.
Also, BazelProfile's members are no longer mutable, meaning we can safely precompute the values without fear of staleness.