Skip to content

Commit

Permalink
src: make BaseObject iteration order deterministic
Browse files Browse the repository at this point in the history
Previously we just rely on the unordered_set order to iterate over
the BaseObjects, which is not deterministic.

The iteration is only used in printing, verification, and snapshot
generation. In the first two cases the performance overhead of
sorting does not matter because they are only used for debugging.
In the last case the determinism is more important than the trivial
overhead of sorting. So this patch makes the iteration deterministic
by sorting the set first, as what is already being done when we
drain the queue.

PR-URL: #48702
Refs: nodejs/build#3043
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
  • Loading branch information
joyeecheung authored and juanarbol committed Jul 13, 2023
1 parent 67a1018 commit a9a4b73
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 2 deletions.
4 changes: 3 additions & 1 deletion src/cleanup_queue-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ void CleanupQueue::Remove(Callback cb, void* arg) {

template <typename T>
void CleanupQueue::ForEachBaseObject(T&& iterator) const {
for (const auto& hook : cleanup_hooks_) {
std::vector<CleanupHookCallback> callbacks = GetOrdered();

for (const auto& hook : callbacks) {
BaseObject* obj = GetBaseObject(hook);
if (obj != nullptr) iterator(obj);
}
Expand Down
9 changes: 8 additions & 1 deletion src/cleanup_queue.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@

namespace node {

void CleanupQueue::Drain() {
std::vector<CleanupQueue::CleanupHookCallback> CleanupQueue::GetOrdered()
const {
// Copy into a vector, since we can't sort an unordered_set in-place.
std::vector<CleanupHookCallback> callbacks(cleanup_hooks_.begin(),
cleanup_hooks_.end());
Expand All @@ -20,6 +21,12 @@ void CleanupQueue::Drain() {
return a.insertion_order_counter_ > b.insertion_order_counter_;
});

return callbacks;
}

void CleanupQueue::Drain() {
std::vector<CleanupHookCallback> callbacks = GetOrdered();

for (const CleanupHookCallback& cb : callbacks) {
if (cleanup_hooks_.count(cb) == 0) {
// This hook was removed from the `cleanup_hooks_` set during another
Expand Down
2 changes: 2 additions & 0 deletions src/cleanup_queue.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <cstddef>
#include <cstdint>
#include <unordered_set>
#include <vector>

#include "memory_tracker.h"

Expand Down Expand Up @@ -66,6 +67,7 @@ class CleanupQueue : public MemoryRetainer {
uint64_t insertion_order_counter_;
};

std::vector<CleanupHookCallback> GetOrdered() const;
inline BaseObject* GetBaseObject(const CleanupHookCallback& callback) const;

// Use an unordered_set, so that we have efficient insertion and removal.
Expand Down

0 comments on commit a9a4b73

Please sign in to comment.