Skip to content
This repository has been archived by the owner on Oct 30, 2021. It is now read-only.

Expand 'mask' bitmask space from unsigned short to uint32_t (16=>32 bits) #82

Merged
merged 4 commits into from
Jan 30, 2017
Merged

Expand 'mask' bitmask space from unsigned short to uint32_t (16=>32 bits) #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
944ce1d
Add unit test to demonstrate bitmask overflow => allocation fail
yhahn Jan 29, 2017
f634f86
Test blows up with just 10k grids, go with that
yhahn Jan 29, 2017
754ada7
Increase bitspace for mask from unsigned short (16 bits) to uint32_t …
yhahn Jan 29, 2017
90e593e
Update tape ht @mattficke
yhahn Jan 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"protozero": "~1.4.5"
},
"devDependencies": {
"tape": "3.0.x"
"tape": "^4.6.3"
},
"main": "./index.js",
"scripts": {
Expand Down
19 changes: 9 additions & 10 deletions src/binding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,7 @@ struct PhrasematchSubq {
uint64_t phrase;
unsigned short idx;
unsigned short zoom;
unsigned short mask;
uint32_t mask;
};

struct Cover {
Expand All @@ -851,19 +851,19 @@ struct Cover {
unsigned short y;
unsigned short score;
unsigned short idx;
unsigned short mask;
uint32_t mask;
unsigned distance;
double scoredist;
};

struct Context {
std::vector<Cover> coverList;
unsigned mask;
uint32_t mask;
double relev;

Context(Context const& c) = default;
Context(Cover && cov,
unsigned mask,
uint32_t mask,
double relev)
: coverList(),
mask(mask),
Expand All @@ -877,7 +877,7 @@ struct Context {
return *this;
}
Context(std::vector<Cover> && cl,
unsigned mask,
uint32_t mask,
double relev)
: coverList(std::move(cl)),
mask(mask),
Expand Down Expand Up @@ -1187,8 +1187,7 @@ void coalesceSingle(uv_work_t* req) {
added++;

double relev = cover.relev;
// TODO correct default mask value?
unsigned mask = 0;
uint32_t mask = 0;
contexts.emplace_back(std::move(cover),mask,relev);
}

Expand Down Expand Up @@ -1312,7 +1311,7 @@ void coalesceMulti(uv_work_t* req) {
std::vector<Cover> covers;
covers.reserve(stackSize);
covers.push_back(cover);
unsigned context_mask = cover.mask;
uint32_t context_mask = cover.mask;
double context_relev = cover.relev;

for (unsigned a = 0; a < zCacheSize; a++) {
Expand All @@ -1323,7 +1322,7 @@ void coalesceMulti(uv_work_t* req) {
static_cast<uint64_t>(std::floor(cover.y/s));
pit = coalesced.find(pxy);
if (pit != coalesced.end()) {
unsigned lastMask = 0;
uint32_t lastMask = 0;
double lastRelev = 0.0;
for (auto const& parents : pit->second) {
for (auto const& parent : parents.coverList) {
Expand Down Expand Up @@ -1475,7 +1474,7 @@ NAN_METHOD(Cache::coalesce) {

subq.weight = jsStack->Get(Nan::New("weight").ToLocalChecked())->NumberValue();
subq.phrase = jsStack->Get(Nan::New("phrase").ToLocalChecked())->IntegerValue();
subq.mask = static_cast<unsigned short>(jsStack->Get(Nan::New("mask").ToLocalChecked())->IntegerValue());
subq.mask = static_cast<std::uint32_t>(jsStack->Get(Nan::New("mask").ToLocalChecked())->IntegerValue());

// JS cache reference => cpp
Local<Object> cache = Local<Object>::Cast(jsStack->Get(Nan::New("cache").ToLocalChecked()));
Expand Down
99 changes: 99 additions & 0 deletions test/coalesce.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,3 +690,102 @@ test('coalesce args', function(assert) {
});
});
})();

// Mask overflow
(function() {
var a = new Cache('a', 0);
var b = new Cache('b', 0);
var c = new Cache('c', 0);

var grids = [];
for (var i = 1; i < 10e3; i++) grids.push(Grid.encode({
id: i,
x: 0,
y: 0,
relev: 1,
score: 1
}));
a._set('grid', 0, 1, grids);

b._set('grid', 0, 1, [
Grid.encode({
id: 1,
x: 0,
y: 0,
relev: 1,
score: 1
})
]);
c._set('grid', 0, 1, [
Grid.encode({
id: 1,
x: 0,
y: 0,
relev: 1,
score: 1
})
]);

test('coalesceMulti mask safe', function(assert) {
assert.comment('start coalesce (mask: 2)');
coalesce([{
cache: a,
mask: 1 << 2,
idx: 0,
zoom: 0,
weight: 0.33,
phrase: 1
}, {
cache: b,
mask: 1 << 0,
idx: 1,
zoom: 0,
weight: 0.33,
phrase: 1
}, {
cache: c,
mask: 1 << 1,
idx: 2,
zoom: 0,
weight: 0.33,
phrase: 1
}], {}, function(err, res) {
assert.ifError(err);
assert.equal(res.length, 1, 'res length = 1');
assert.deepEqual(res[0].map(function(f) { return f.id; }), [1, 1, 1], '0.relev = 0.99');
assert.end();
});
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testcase below works by

  • Adding 10k features with the same x/y coordinates to a gridcache entry
  • Layering two features on top of causing each to inherit the 10k features into their respective cover lists

Normally as each of those features incorporate parent features into their contexts, they would do a mask check making sure that they don't add two features that both correspond to the same term in a query. When a mask overflow occurs in master it results in an empty mask which can always be combined with any other feature. This leads to the child features inheriting all 10k parent features and leads to OOM.

test('coalesceMulti mask overflow', function(assert) {
assert.comment('start coalesce (mask: 18)');
coalesce([{
cache: a,
mask: 1 << 18,
idx: 0,
zoom: 0,
weight: 0.33,
phrase: 1
}, {
cache: b,
mask: 1 << 0,
idx: 1,
zoom: 0,
weight: 0.33,
phrase: 1
}, {
cache: c,
mask: 1 << 1,
idx: 2,
zoom: 0,
weight: 0.33,
phrase: 1
}], {}, function(err, res) {
assert.ifError(err);
assert.equal(res.length, 1, 'res length = 1');
assert.deepEqual(res[0].map(function(f) { return f.id; }), [1, 1, 1], '0.relev = 0.99');
assert.end();
});
});
})();