Skip to content

Commit

Permalink
src: make ELDHistogram a HandleWrap
Browse files Browse the repository at this point in the history
This simplifies the implementation of ELDHistogram a bit,
and more generally allows us to have weak JS references
associated with `HandleWrap`s.

PR-URL: #29317
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and BridgeAR committed Sep 25, 2019
1 parent 7dd897f commit 67aa5ef
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 34 deletions.
1 change: 1 addition & 0 deletions src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ namespace node {
#define NODE_ASYNC_NON_CRYPTO_PROVIDER_TYPES(V) \
V(NONE) \
V(DNSCHANNEL) \
V(ELDHISTOGRAM) \
V(FILEHANDLE) \
V(FILEHANDLECLOSEREQ) \
V(FSEVENTWRAP) \
Expand Down
16 changes: 13 additions & 3 deletions src/handle_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,17 @@ void HandleWrap::Close(Local<Value> close_callback) {
}


void HandleWrap::MakeWeak() {
persistent().SetWeak(
this,
[](const v8::WeakCallbackInfo<HandleWrap>& data) {
HandleWrap* handle_wrap = data.GetParameter();
handle_wrap->persistent().Reset();
handle_wrap->Close();
}, v8::WeakCallbackType::kParameter);
}


void HandleWrap::MarkAsInitialized() {
env()->handle_wrap_queue()->PushBack(this);
state_ = kInitialized;
Expand Down Expand Up @@ -115,15 +126,14 @@ void HandleWrap::OnClose(uv_handle_t* handle) {
HandleScope scope(env->isolate());
Context::Scope context_scope(env->context());

// The wrap object should still be there.
CHECK_EQ(wrap->persistent().IsEmpty(), false);
CHECK_EQ(wrap->state_, kClosing);

wrap->state_ = kClosed;

wrap->OnClose();

if (wrap->object()->Has(env->context(), env->handle_onclose_symbol())
if (!wrap->persistent().IsEmpty() &&
wrap->object()->Has(env->context(), env->handle_onclose_symbol())
.FromMaybe(false)) {
wrap->MakeCallback(env->handle_onclose_symbol(), 0, nullptr);
}
Expand Down
2 changes: 2 additions & 0 deletions src/handle_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ class HandleWrap : public AsyncWrap {
static v8::Local<v8::FunctionTemplate> GetConstructorTemplate(
Environment* env);

void MakeWeak(); // This hides BaseObject::MakeWeak()

protected:
HandleWrap(Environment* env,
v8::Local<v8::Object> object,
Expand Down
39 changes: 13 additions & 26 deletions src/node_perf.cc
Original file line number Diff line number Diff line change
Expand Up @@ -477,31 +477,18 @@ static void ELDHistogramNew(const FunctionCallbackInfo<Value>& args) {
ELDHistogram::ELDHistogram(
Environment* env,
Local<Object> wrap,
int32_t resolution) : BaseObject(env, wrap),
int32_t resolution) : HandleWrap(env,
wrap,
reinterpret_cast<uv_handle_t*>(&timer_),
AsyncWrap::PROVIDER_ELDHISTOGRAM),
Histogram(1, 3.6e12),
resolution_(resolution) {
MakeWeak();
timer_ = new uv_timer_t();
uv_timer_init(env->event_loop(), timer_);
timer_->data = this;
uv_timer_init(env->event_loop(), &timer_);
}

void ELDHistogram::CloseTimer() {
if (timer_ == nullptr)
return;

env()->CloseHandle(timer_, [](uv_timer_t* handle) { delete handle; });
timer_ = nullptr;
}

ELDHistogram::~ELDHistogram() {
Disable();
CloseTimer();
}

void ELDHistogramDelayInterval(uv_timer_t* req) {
ELDHistogram* histogram =
reinterpret_cast<ELDHistogram*>(req->data);
void ELDHistogram::DelayIntervalCallback(uv_timer_t* req) {
ELDHistogram* histogram = ContainerOf(&ELDHistogram::timer_, req);
histogram->RecordDelta();
TRACE_COUNTER1(TRACING_CATEGORY_NODE2(perf, event_loop),
"min", histogram->Min());
Expand Down Expand Up @@ -537,21 +524,21 @@ bool ELDHistogram::RecordDelta() {
}

bool ELDHistogram::Enable() {
if (enabled_) return false;
if (enabled_ || IsHandleClosing()) return false;
enabled_ = true;
prev_ = 0;
uv_timer_start(timer_,
ELDHistogramDelayInterval,
uv_timer_start(&timer_,
DelayIntervalCallback,
resolution_,
resolution_);
uv_unref(reinterpret_cast<uv_handle_t*>(timer_));
uv_unref(reinterpret_cast<uv_handle_t*>(&timer_));
return true;
}

bool ELDHistogram::Disable() {
if (!enabled_) return false;
if (!enabled_ || IsHandleClosing()) return false;
enabled_ = false;
uv_timer_stop(timer_);
uv_timer_stop(&timer_);
return true;
}

Expand Down
8 changes: 3 additions & 5 deletions src/node_perf.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,14 +123,12 @@ class GCPerformanceEntry : public PerformanceEntry {
PerformanceGCKind gckind_;
};

class ELDHistogram : public BaseObject, public Histogram {
class ELDHistogram : public HandleWrap, public Histogram {
public:
ELDHistogram(Environment* env,
Local<Object> wrap,
int32_t resolution);

~ELDHistogram() override;

bool RecordDelta();
bool Enable();
bool Disable();
Expand All @@ -149,13 +147,13 @@ class ELDHistogram : public BaseObject, public Histogram {
SET_SELF_SIZE(ELDHistogram)

private:
void CloseTimer();
static void DelayIntervalCallback(uv_timer_t* req);

bool enabled_ = false;
int32_t resolution_ = 0;
int64_t exceeds_ = 0;
uint64_t prev_ = 0;
uv_timer_t* timer_;
uv_timer_t timer_;
};

} // namespace performance
Expand Down
1 change: 1 addition & 0 deletions test/sequential/test-async-wrap-getasyncid.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const { getSystemErrorName } = require('util');
delete providers.KEYPAIRGENREQUEST;
delete providers.HTTPCLIENTREQUEST;
delete providers.HTTPINCOMINGMESSAGE;
delete providers.ELDHISTOGRAM;

const objKeys = Object.keys(providers);
if (objKeys.length > 0)
Expand Down

0 comments on commit 67aa5ef

Please sign in to comment.