phongn commented on PR #13257: URL: https://github.com/apache/trafficserver/pull/13257#issuecomment-5700190926
Correcting something I told you in my earlier reply, since the claim is wrong and it is load-bearing for a decision you may make later. On the destructor thread, I wrote: > Making it genuinely safe means giving the compressor `stripe->mutex` and requiring the destructor to hold it, which changes production scheduling behaviour (the compressor would then contend and reschedule on lock miss) for a path production never takes The stripe mutex is exactly the wrong mutex for this, and not for the reason I gave. `Mutex_unlock()` only decrements `nthread_holding` and releases the underlying lock when that reaches zero (`include/iocore/eventsystem/Lock.h:363-368`). `compress_entries()` relies on actually dropping the stripe lock around the codec call: it takes the lock on entry, `MUTEX_UNTAKE_LOCK`s before compressing, and retakes it afterwards. If the continuation were dispatched already holding `stripe->mutex`, that inner untake would take the count from 2 to 1 rather than releasing, so every codec call would run with the stripe locked. That is a throughput regression on the RAM cache hit path, which is considerably worse than the contend-and-reschedule cost I described. The right shape is the one used elsewhere in the tree: give `RamCacheCLFUSCompressor` its own `new_ProxyMutex()`, keep the `Event *` that `schedule_every()` returns, and cancel it under that mutex in the destructor. `QUICPacketHandler`'s `_collector_event` and `PreWarmManager`'s `_tick_event` both do this. That also removes the reason the destructor currently only documents its precondition: with a retained `Event *` it can enforce it. Worth being explicit about why the cancel is needed at all, which I was vague about: `EventProcessor::schedule` only copies a mutex onto the event if the continuation has one (`UnixEventProcessor.cc:704-706`), and the lock helpers treat a null mutex as already acquired (`Lock.h:518-519`). So `RamCacheCLFUSCompressor`, having no mutex, is dispatched with no synchronisation against anything at all today. I have not made that change here — it is a behavioural change to production scheduling and belongs with the shared-compression refactor, where the compressor continuation moves anyway. I have queued it there, and corrected the comment in `~RamCacheCLFUS()` that repeated the same bad advice. My conclusion for this PR is unchanged: document the precondition rather than ship a cancel that looks safe and is not. -- 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]
