From b0f333b0340c0b34c014519d9b26901766b2053a Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 23 Mar 2016 16:39:16 -0700 Subject: [PATCH 1/4] Cache repository sync return values Affects: git_blob_owner git_commit_owner git_filter_source_repo git_index_owner git_object_owner git_reference_owner git_remote_owner git_revwalk_repository git_submodule_owner git_tag_owner git_tree_owner --- generate/input/descriptor.json | 1 + generate/scripts/helpers.js | 1 + generate/templates/partials/sync_function.cc | 41 ++++++++++++++++++- generate/templates/templates/class_content.cc | 10 +++++ generate/templates/templates/class_header.h | 8 ++++ 5 files changed, 60 insertions(+), 1 deletion(-) diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 29f9e2ce4..46503da33 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -1767,6 +1767,7 @@ "ignore": true }, "repository": { + "cacheResult": true, "functions": { "git_repository__cleanup": { "ignore": true diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index 6e1f33613..6f3341209 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -179,6 +179,7 @@ var Helpers = { typeDef.filename = typeDef.typeName; typeDef.isLibgitType = true; typeDef.dependencies = []; + typeDef.cacheResult = Boolean(typeDefOverrides.cacheResult); typeDef.selfFreeing = Boolean(typeDefOverrides.selfFreeing); if (typeDefOverrides.freeFunctionName) { diff --git a/generate/templates/partials/sync_function.cc b/generate/templates/partials/sync_function.cc index ada0de62f..976061e3d 100644 --- a/generate/templates/partials/sync_function.cc +++ b/generate/templates/partials/sync_function.cc @@ -2,6 +2,11 @@ {%partial doc .%} NAN_METHOD({{ cppClassName }}::{{ cppFunctionName }}) { Nan::EscapableHandleScope scope; + +{%if return.cacheResult %} + {{ cppClassName }} *thisObj = Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This()); + return info.GetReturnValue().Set(scope.Escape(Nan::New(thisObj->{{ cppFunctionName }}_cachedResult))); +{% else %} {%partial guardArguments .%} {%each .|returnsInfo 'true' as _return %} @@ -35,7 +40,7 @@ if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL {% endif %} giterr_clear(); - + { LockMaster lockMaster(true{%each args|argsInfo as arg %} {%if arg.cType|isPointer%}{%if not arg.isReturn%} @@ -125,4 +130,38 @@ if (Nan::ObjectWrap::Unwrap<{{ cppClassName }}>(info.This())->GetValue() != NULL {%endif%} {%endif%} } +{%endif%} } + +{%if return.cacheResult %} +void {{ cppClassName }}::{{ cppFunctionName }}_cache() { + if (!raw) { + {{ cppFunctionName }}_cachedResult.Reset(Nan::Null()); + return; + } + + LockMaster lockMaster(true{%each args|argsInfo as arg %} + {%if arg.cType|isPointer%}{%if not arg.isReturn%} + ,{%if arg.isSelf %} + raw + {%endif%} + {%endif%}{%endif%} + {%endeach%}); + + {{ return.cType }} result = {{ cFunctionName }}( + {%each args|argsInfo as arg %} + {%if arg.isSelf %} + raw + {%endif%} + {%endeach%} + ); + + Local to; + + {%each .|returnsInfo as _return %} + {%partial convertToV8 _return %} + {%endeach%} + + {{ cppFunctionName }}_cachedResult.Reset(to); +} +{% endif %} diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 312c2ef28..993b11663 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -44,6 +44,16 @@ using namespace node; NonSelfFreeingConstructedCount++; } + + {%each functions as function%} + {%if not function.ignore %} + {%if not function.isAsync %} + {%if function.return.cacheResult %} + {{ function.cppFunctionName }}_cache(); // populate cached value + {%endif%} + {%endif%} + {%endif%} + {%endeach%} } {{ cppClassName }}::~{{ cppClassName }}() { diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index cc77f52b3..9e09ad731 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -141,6 +141,14 @@ class {{ cppClassName }} : public Nan::ObjectWrap { private: {{ function.cppFunctionName }}Baton *baton; }; + {%else%} + {%if function.return.cacheResult %} + // For simple sync functions that return a wrapped object and pass `raw` + // as the the only parameter to libgit2, we cache the results. + // CopyablePersistentTraits are used to get the reset-on-destruct behavior. + void {{ function.cppFunctionName }}_cache(); + Nan::Persistent > {{ function.cppFunctionName }}_cachedResult; + {%endif%} {%endif%} static NAN_METHOD({{ function.cppFunctionName }}); From c23d58fd62a1429d35bfcc1afdc9cf32879853d2 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Tue, 22 Mar 2016 17:18:02 -0700 Subject: [PATCH 2/4] Add test for owner caching --- test/tests/commit.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/tests/commit.js b/test/tests/commit.js index e770a0c1e..c457dc01d 100644 --- a/test/tests/commit.js +++ b/test/tests/commit.js @@ -404,6 +404,12 @@ describe("Commit", function() { assert.ok(owner instanceof Repository); }); + it("caches its owner", function() { + var owner = this.commit.owner(); + var ownerAgain = this.commit.owner(); + assert.ok(owner === ownerAgain); + }); + it("can walk its repository's history", function(done) { var historyCount = 0; var expectedHistoryCount = 364; From cd21a0f3421eea03821b14c18d2afa27ecf21350 Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Thu, 24 Mar 2016 00:03:33 -0700 Subject: [PATCH 3/4] Pass id instead of object to createBranch --- test/tests/submodule.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tests/submodule.js b/test/tests/submodule.js index 002b4ef12..d4f9a9f70 100644 --- a/test/tests/submodule.js +++ b/test/tests/submodule.js @@ -134,7 +134,7 @@ describe("Submodule", function() { return reference.peel(NodeGit.Object.TYPE.COMMIT); }) .then(function(commit) { - return submoduleRepo.createBranch("master", commit); + return submoduleRepo.createBranch("master", commit.id()); }) .then(function() { return submodule.addFinalize(); From 5d57efb8cbd057c4c78958d32383bdc9c144771d Mon Sep 17 00:00:00 2001 From: Stjepan Rajko Date: Wed, 23 Mar 2016 16:44:57 -0700 Subject: [PATCH 4/4] Enforce 1-1 correspondence between repositories and wrappers. --- generate/input/descriptor.json | 1 + generate/scripts/helpers.js | 1 + .../templates/manual/include/functions/noop.h | 8 ++++ .../templates/manual/src/functions/noop.cc | 4 ++ generate/templates/templates/binding.gyp | 1 + generate/templates/templates/class_content.cc | 44 +++++++++++++++++-- generate/templates/templates/class_header.h | 5 +++ test/tests/commit.js | 34 ++++++++++++-- 8 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 generate/templates/manual/include/functions/noop.h create mode 100644 generate/templates/manual/src/functions/noop.cc diff --git a/generate/input/descriptor.json b/generate/input/descriptor.json index 46503da33..d09810e12 100644 --- a/generate/input/descriptor.json +++ b/generate/input/descriptor.json @@ -1768,6 +1768,7 @@ }, "repository": { "cacheResult": true, + "oneToOne": true, "functions": { "git_repository__cleanup": { "ignore": true diff --git a/generate/scripts/helpers.js b/generate/scripts/helpers.js index 6f3341209..e5e8cd917 100644 --- a/generate/scripts/helpers.js +++ b/generate/scripts/helpers.js @@ -181,6 +181,7 @@ var Helpers = { typeDef.dependencies = []; typeDef.cacheResult = Boolean(typeDefOverrides.cacheResult); typeDef.selfFreeing = Boolean(typeDefOverrides.selfFreeing); + typeDef.oneToOne = Boolean(typeDefOverrides.oneToOne); if (typeDefOverrides.freeFunctionName) { typeDef.freeFunctionName = typeDefOverrides.freeFunctionName; diff --git a/generate/templates/manual/include/functions/noop.h b/generate/templates/manual/include/functions/noop.h new file mode 100644 index 000000000..35a14f11f --- /dev/null +++ b/generate/templates/manual/include/functions/noop.h @@ -0,0 +1,8 @@ +#ifndef NOOP_FUNCTIONS +#define NOOP_FUNCTIONS + +#include + +void noop(const Nan::WeakCallbackInfo &data); + +#endif diff --git a/generate/templates/manual/src/functions/noop.cc b/generate/templates/manual/src/functions/noop.cc new file mode 100644 index 000000000..a34579ebf --- /dev/null +++ b/generate/templates/manual/src/functions/noop.cc @@ -0,0 +1,4 @@ + +#include "../../include/functions/noop.h" + +void noop(const Nan::WeakCallbackInfo &data) {} diff --git a/generate/templates/templates/binding.gyp b/generate/templates/templates/binding.gyp index 9a2813579..66412c293 100644 --- a/generate/templates/templates/binding.gyp +++ b/generate/templates/templates/binding.gyp @@ -18,6 +18,7 @@ "src/promise_completion.cc", "src/wrapper.cc", "src/functions/copy.cc", + "src/functions/noop.cc", "src/functions/sleep_for_ms.cc", "src/convenient_patch.cc", "src/convenient_hunk.cc", diff --git a/generate/templates/templates/class_content.cc b/generate/templates/templates/class_content.cc index 993b11663..bee79166e 100644 --- a/generate/templates/templates/class_content.cc +++ b/generate/templates/templates/class_content.cc @@ -10,6 +10,7 @@ extern "C" { #include "../include/lock_master.h" #include "../include/functions/copy.h" +#include "../include/functions/noop.h" #include "../include/{{ filename }}.h" #include "../include/functions/sleep_for_ms.h" @@ -25,6 +26,14 @@ using namespace node; {% if cType %} {{ cppClassName }}::{{ cppClassName }}({{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) { + {% if oneToOne %} + if(instances.find(raw) != instances.end()) { + Nan::ThrowError("{{ cppClassName }} constructor called on an already wrapped {{ cType }} object"); + return; + } + selfFreeing = true; + {% endif %} + if (shouldDuplicate) { {% if shouldAlloc %} this->raw = ({{ cType }} *)malloc(sizeof({{ cType }})); @@ -44,7 +53,7 @@ using namespace node; NonSelfFreeingConstructedCount++; } - + {%each functions as function%} {%if not function.ignore %} {%if not function.isAsync %} @@ -62,7 +71,7 @@ using namespace node; {{ freeFunctionName }}(this->raw); SelfFreeingInstanceCount--; - this->raw = NULL; + ClearValue(); } {% endif %} @@ -136,8 +145,24 @@ using namespace node; Local {{ cppClassName }}::New(const {{ cType }} *raw, bool selfFreeing, bool shouldDuplicate) { Nan::EscapableHandleScope scope; + + {% if oneToOne %} + auto mapElement = instances.find(const_cast<{{ cType }} *>(raw)); + if(mapElement != instances.end()) { + return scope.Escape(Nan::New(*mapElement->second)); + } + {% endif %} + Local argv[3] = { Nan::New((void *)raw), Nan::New(selfFreeing), Nan::New(shouldDuplicate) }; - return scope.Escape(Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 3, argv).ToLocalChecked()); + Local instance = Nan::NewInstance(Nan::New({{ cppClassName }}::constructor_template), 3, argv).ToLocalChecked(); + {% if oneToOne %} + Nan::Persistent *persistent = new Nan::Persistent(instance); + persistent->SetWeak((void *)NULL, noop, Nan::WeakCallbackType::kParameter); + if (raw) { + instances[const_cast(raw)] = persistent; + } + {% endif %} + return scope.Escape(instance); } NAN_METHOD({{ cppClassName }}::GetSelfFreeingInstanceCount) { @@ -153,6 +178,14 @@ using namespace node; } void {{ cppClassName }}::ClearValue() { + {% if oneToOne %} + auto mapElement = instances.find(raw); + if(mapElement != instances.end()) { + delete mapElement->second; + instances.erase(mapElement); + } + {% endif %} + this->raw = NULL; } @@ -192,5 +225,10 @@ using namespace node; Nan::Persistent {{ cppClassName }}::constructor_template; {% endif %} +{%if oneToOne %} +// map to ensure one wrapper per libgit2 instance +std::map<{{ cType }} *, Nan::Persistent *> {{ cppClassName }}::instances; +{%endif%} + int {{ cppClassName }}::SelfFreeingInstanceCount; int {{ cppClassName }}::NonSelfFreeingConstructedCount; diff --git a/generate/templates/templates/class_header.h b/generate/templates/templates/class_header.h index 9e09ad731..ce4f95ac0 100644 --- a/generate/templates/templates/class_header.h +++ b/generate/templates/templates/class_header.h @@ -1,6 +1,7 @@ #ifndef {{ cppClassName|upper }}_H #define {{ cppClassName|upper }}_H #include +#include #include #include #include @@ -36,6 +37,10 @@ using namespace node; using namespace v8; class {{ cppClassName }} : public Nan::ObjectWrap { + {%if oneToOne %} + // map to ensure one wrapper per libgit2 instance + static std::map<{{ cType }} *, Nan::Persistent *> instances; + {%endif%} public: static Nan::Persistent constructor_template; diff --git a/test/tests/commit.js b/test/tests/commit.js index c457dc01d..d19813467 100644 --- a/test/tests/commit.js +++ b/test/tests/commit.js @@ -404,10 +404,36 @@ describe("Commit", function() { assert.ok(owner instanceof Repository); }); - it("caches its owner", function() { - var owner = this.commit.owner(); - var ownerAgain = this.commit.owner(); - assert.ok(owner === ownerAgain); + it("caches its owner", function(done) { + var objects = {}; + + reinitialize(objects) + .then(function() { + var owner = objects.commit.owner(); + var ownerAgain = objects.commit.owner(); + assert.ok(owner === ownerAgain); + assert.ok(owner === objects.repository); + + // make sure the owner gets freed at the correct time + garbageCollect(); + var Repository = NodeGit.Repository; + var startCount = Repository.getSelfFreeingInstanceCount(); + owner = ownerAgain = objects.repository = null; + + setTimeout(function() { + garbageCollect(); + // the commit should still hold the repository + assert.equal(startCount, Repository.getSelfFreeingInstanceCount()); + objects.commit = null; + // without the setTimeout, not seeing a free + setTimeout(function() { + garbageCollect(); + assert.equal(startCount - 1, + Repository.getSelfFreeingInstanceCount()); + done(); + }, 10); + }, 10); + }); }); it("can walk its repository's history", function(done) {