andygrove commented on PR #6166: URL: https://github.com/apache/datafusion-comet/pull/6166#issuecomment-5821703602
Thanks @mbutrovich and @comphead. I've gone with @comphead's suggestion. The PR now only deletes `IN_TRACK` from `main`'s version, so `LOCAL_DRIFT.try_with` is the only thread-local access, and std's own state for `LOCAL_DRIFT` does what the hand-rolled phase did. The inline comments on `STATE` and `SettleOnExit` no longer apply, since that code is gone. I checked the variant on both platforms before switching. On macOS it makes one TLV thunk call in each `GlobalAlloc` method, as the first revision did, and an interleaved A/B over the `alloc_overhead` cases put it level with the first revision and about 1.5 ns per alloc/free pair ahead of `main`. For x86_64 Linux I compiled a standalone copy with the flags Cargo uses for `libcomet`, which gives one `__tls_get_addr` call per method, against two on `main`. That needed checking on its own, because a cdylib-only build of the same code uses the local-dynamic TLS model and makes a single call even on `main`. `libcomet` is also an rlib, which keeps these thread-locals exported and so general-dynamic. It also means Cargo never applies the release profile's `lto = "thin"` to `libcomet`. I haven't re-run Q21 on this revision, and the description says which numbers come from the first one. The tests pass on the new head with the system allocator, jemalloc and mimalloc, the description is updated for #6162, and CI is running again now that Preflight got past the mirror. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
