-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add PoolingAllocatorMetrics #11490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PoolingAllocatorMetrics #11490
Conversation
| Ok(result) => return Ok(result), | ||
| Err(e) => e, | ||
| }; | ||
| self.live_memories.fetch_add(1, Ordering::AcqRel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence about using Ordering::Relaxed for these, but if someone ends up using metrics in actual business logic (like server backpressure) it would be a pretty nasty surprise for these to underflow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually opt towards using Relaxed here since there's explicitly no data dependency with anything else in the system (e.g. this isn't looking for memory effects of another operation or publishing its own memory effects). Could you detail more about the underflow point though? How would atomic ordering affect that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I don't really have a good grasp of relaxed atomic semantics so I just pessimistically assume that a concurrent thread would be able to go back in time and kill its own grandparent or whatever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh nah makes sense. I'm by no means an expert but I'm confident enough in my understanding that if atomics are used for things like metric purposes it's fine to use relaxed. Relaxed ordering, AFAIK, basically means that you get no guarantees about other memory locations in the program when you observe a value. For the memory location in question, though, everything is still just as atomic as you'd expect (e.g. fetch-and-add still does that, it doesn't like switch to non-atomics). This happens because relaxed atomics can be reordered around each other by the compiler, so you can't for example say "I saw N memories in use, therefore I must see N+1 tables in use next", that's not valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah makes sense. I guess I'm not entirely clear on what makes increment_component_instance_count different here (above) that it would need AcqRel?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh it's not, the main difference is I saw this here and no one saw it there most likely. There's no need for that to use AcqRel either and IMO it should also be using Relaxed
| Ok(result) => return Ok(result), | ||
| Err(e) => e, | ||
| }; | ||
| self.live_memories.fetch_add(1, Ordering::AcqRel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd actually opt towards using Relaxed here since there's explicitly no data dependency with anything else in the system (e.g. this isn't looking for memory effects of another operation or publishing its own memory effects). Could you detail more about the underflow point though? How would atomic ordering affect that?
crates/wasmtime/src/runtime/vm/instance/allocator/pooling/metrics.rs
Outdated
Show resolved
Hide resolved
b2a427f to
fc3f429
Compare
|
While the vet error is spurious, the miri failure looks legitimate. |
|
This does indeed fail under miri locally: wasmtime/crates/wasmtime/src/runtime/vm/instance/allocator/pooling/metrics.rs Lines 76 to 77 in fc3f429
Presumably I can work around this by overriding some default config, but is it a bug that the defaults don't work with miri? |
|
Oh well I guess that's pretty straightforward: wasmtime/crates/environ/src/tunables.rs Lines 149 to 150 in fc3f429
Any preference on how to fix this?
|
|
The main test suite has a helper function for a "small pool config" which could be modeled here as well, but this test isn't too interesting for miri so it's also fine to slap a |
This exposes some basic runtime metrics derived from the internal state of a `PoolingInstanceAllocator`. Two new atomics were added to PoolingInstanceAllocator: `live_memories` and `live_tables`. While these counts could be derived from existing state it would require acquiring mutexes on some inner state.
This exposes some basic runtime metrics derived from the internal state of a `PoolingInstanceAllocator`. Two new atomics were added to PoolingInstanceAllocator: `live_memories` and `live_tables`. While these counts could be derived from existing state it would require acquiring mutexes on some inner state.
This exposes some basic runtime metrics derived from the internal state of a
PoolingInstanceAllocator.Two new atomics were added to PoolingInstanceAllocator:
live_memoriesandlive_tables. While these counts could be derived from existing state it would require acquiring mutexes on some inner state.Fixes #11449