[wip] Refactor carmen-cache by apendleton · Pull Request #116 · mapbox/carmen-cache

apendleton · 2018-02-16T21:24:46Z

This PR reflects the work-in-progress refactor effort described in #115 .

…sce is done now and the util files are started

…p/hpp files; update other files for appropriate includes"

…sed parameters, which I'm squashing with pragmas since they're mandated by outside libraries

…reachable code (mostly extra returns at the ends of functions)

…he-split

…lly-committed package-lock.json

…he-split

…comments for the C++-internal parts of coalesce, as well as node_util

apendleton

@aaaandrea here are some notes on the docs for MemoryCache, RocksDBCache, and cpp_util. Sorry for the wall of text 😄

apendleton · 2018-02-22T21:30:18Z

src/memorycache.cpp

+ * @name pack
+ * @memberof MemoryCache
+ * @param {String}, filename
+ * @returns {String}, filename


This doesn't return anything:

info.GetReturnValue().Set(Nan::Undefined());

apendleton · 2018-02-22T21:31:13Z

src/memorycache.cpp

+}
+
+/**
+ * lists the data in the memory cache object


This only lists the keys in the store, not the values; "data" is ambiguous

apendleton · 2018-02-22T21:33:41Z

src/memorycache.cpp

+}
+
+/**
+ * replaces the data in the object


This function doesn't set all the data, just the data for a given key. It might replace, or it might append to what's already there, depending on the value of the append argument

apendleton · 2018-02-22T21:34:52Z

src/memorycache.cpp

+ * @name set
+ * @memberof MemoryCache
+ * @param {String} id
+ * @param {Array}, data


It takes up to four arguments; the third and fourth are optional (append is a boolean for whether to append or replace and languages is a language ID array). Also it would be good to say what kind of array the data is (an array of numbers), and that each number represents a grid.

apendleton · 2018-02-22T21:35:07Z

src/memorycache.cpp

+ * @memberof MemoryCache
+ * @param {String} id
+ * @param {Array}, data
+ * @returns {String}


Again, doesn't return anything

apendleton · 2018-02-22T22:03:51Z

src/cpp_util.cpp

+
+namespace carmen {
+
+// derives grid values


Covers are a representation of (relev, score, x, y, feature_id) packed into a single 64-bit integer. This function converts from the packed integer into the full structure where you can access each property.

apendleton · 2018-02-22T22:05:31Z

src/cpp_util.cpp

+    return cover;
+}
+
+// Compute distance between coordinates


This isn't a distance calculation. It converts from the ZXY coordinates of the tile that contains a proximity point to a ZXY at the appropriate zoom level necessary for a given coalesce operation (which may examine grids at multiple zoom levels that might not be the same as the zoom level used to specify the proximity location).

apendleton · 2018-02-22T22:06:55Z

src/cpp_util.cpp

+}
+
+
+// Compute distance between coords + zoom multiplier


Again, doesn't calculate a distance, but rather, calculates a ZXY for appropriate for a given coalesce operation out of the supplied bbox ZXY coordinates.

apendleton · 2018-02-22T22:07:47Z

src/cpp_util.cpp

+    return score > scoredist ? score : scoredist;
+}
+
+// database lookup


This doesn't look anything up, it just opens the file so it's ready for lookups later. We do this when a carmen instance starts. This one is read-write, so we mostly use it for indexing.

apendleton · 2018-02-22T22:08:02Z

src/cpp_util.cpp

+    return status;
+}
+
+// read only database lookup


The same, but read-only (used at runtime).

aaaandrea · 2018-02-22T22:29:42Z

src/normalizationcache.cpp

+ * const cache = require('@mapbox/carmen-cache');
+ * const nc = new cache.NormalizationCache('file.norm.rocksdb', true);
+ *
+ * // for a normalization cache with over the dictionary ['main st', 'main street']


aaaandrea · 2018-02-22T22:36:33Z

src/normalizationcache.cpp

+ *
+ * @name writeBatch
+ * @memberof NormalizationCache
+ * @returns {Array}


add @params

…d ensure that all tape assertions have assertion messages

refs #119 (comment)

Fix LTO on linux

…m in all test environments, and document benchmarking process

boblannon · 2018-03-09T22:05:10Z

src/cpp_util.hpp

+constexpr uint64_t POW2_3 = static_cast<uint64_t>(_pow(2.0, 3));
+constexpr uint64_t POW2_2 = static_cast<uint64_t>(_pow(2.0, 2));
+
+struct PhrasematchSubq {


why not have the typedef doc here?

We could. I'm inclined not to, though. The typedef describes the way JS objects should be formatted in the phrasematches array on the way into coalesce. They come in as JS objects, and then are processed and their contents stored in C++ structs of this type ^. The two types serve the same purpose, but are not the same: aside from being of different types in a technical sense, they also don't have exactly the same fields, either in terms of what they're called or what type they are. Like, languages on the JS side is an array of integers, but that gets converted into a bitfield on the C++ side and stored in a property called langfield. I sort of think conflating the two is a recipe for confusion. (That said, given that, maybe I should call the JS type something different?)

Also, as a practical matter, at the moment we aren't looking for JSDoc entries in headers, and were also trying to keep the cpp_util.* files generally free of JS-isms. Those are both trivially changeable though, so I think they're less compelling arguments than the above.

looks like we crossed in the mail, there. sgtm!

(That said, given that, maybe I should call the JS type something different?)

Would be less confusing if I had API.md open and was browsing C++ code...

Renamed the JS-accessible version in c6d30e5

boblannon

LGTM. Really appreciate the @typedef JSDocs, but it'd be nice to see them alongside associated code, where possible. documentation build takes a glob pattern for inputs, so you could change

documentation build src/*.cpp

to

documentation build src/*.[ch]pp

aarthykc · 2018-03-13T11:24:39Z

src/cpp_util.hpp

+    }
+}
+
+// in general, the key format including language field is <key_text><separator character><8-byte langfield>


…n tests and docs, to avoid confusion with the C++ PhrasematchSubq struct type, which has slightly different properties

apendleton and others added 27 commits February 13, 2018 17:39

Start splitting out carmen-cache functions into separate files; coale…

0286cf7

…sce is done now and the util files are started

add memorycache.cpp; move associated generic functions/structs to utils

b9cf236

mv normalizationcache to own cpp/hpp files; mv rocksdbcache to own cp…

fb67c17

…p/hpp files; update other files for appropriate includes"

add clang diagnostics ignores for external libraries

f222a32

add hello_async.bench template from node-cpp=skel

0b85ce6

Warning smashing: mostly old-style casts (which I fixed) and some unu…

42d646a

…sed parameters, which I'm squashing with pragmas since they're mandated by outside libraries

add hello_object_async.bench from node-cpp-skel

ca42804

More warning squashing: one more unused param pragma, and deleting un…

63f9941

…reachable code (mostly extra returns at the ends of functions)

Get rid of a bunch of extraneous includes

e584176

Apply clang-format fixes

efef399

rm templates

48b6219

Merge branch 'cache-split' of github.com:mapbox/carmen-cache into cac…

b7ef9fd

…he-split

Add pinned documentation.js and docs generation, and remove accidenta…

d9d18fc

…lly-committed package-lock.json

add comments in memcache

86a38c5

Merge branch 'cache-split' of github.com:mapbox/carmen-cache into cac…

5fc0c21

…he-split

add API doc comments to rocksdbcache

c927d8e

update api documentation for each function in memorycache.cpp

2b6ad57

memcache api docs updates

ecd4273

add api docs for rocksdbcache.cpp

a909474

Add JSDoc annotations for NormalizationCache

1d0c1b2

Merge branch 'cache-split' of github.com:mapbox/carmen-cache into cac…

632fb40

…he-split

use yarn, not npm, to install deps

36bdfc3

Add jsdoc entries for the JS-exposed parts coalesce, and regular C++ …

b564a7b

…comments for the C++-internal parts of coalesce, as well as node_util

update memorycache/rocksdbcache api docs; add cpp_util api docs

f30fb53

ad pxytozxy notes

909b1db

make docs run

27be9d6

nest comments for api docs

42fd754

apendleton commented Feb 22, 2018

View reviewed changes

aaaandrea reviewed Feb 22, 2018

View reviewed changes

src/normalizationcache.cpp

*

* @name writeBatch

* @memberof NormalizationCache

* @returns {Array}

Copy link

Contributor

aaaandrea Feb 22, 2018

Choose a reason for hiding this comment

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

add @params

aaaandrea and others added 18 commits February 26, 2018 14:46

format and tidy

fd76c04

add comments to describe tests

af31425

Add tape eslint plugin, switch tests to format required by plugin, an…

f485436

…d ensure that all tape assertions have assertion messages

Drop node 4 from travis build matrix

5057b50

Upgrade to newer mason so binutils installs correctly on travis

a0402f2

update outdated API.md

aed9d5a

source compile rocksbd and bz2

d72a81b

reduce compile concurrency to avoid OOM killer knocking out g++

ca7d004

refs #119 (comment)

correctly limit compiler concurrency

766d3a5

fix to ensure consistent g++ version for that build

045434e

fix typo

c394579

fix typo

c6ba7b3

only use binutils if we are building with clang++

fdd1f64

upgrade to llvm 5.0.1 / mark mason branch as todo

c2c3ee4

Merge pull request #119 from mapbox/mason-deps-from-source

0503234

Fix LTO on linux

Move benchmarks to their own directory, don't enforce timing, run the…

4ce987b

…m in all test environments, and document benchmarking process

Bump to new dev version v0.20.0-cache-split-2 [publish binary]

f0760d9

Fix linting [publish binary]

4041a9b

apendleton mentioned this pull request Mar 2, 2018

Refactor Plan #115

Closed

29 tasks

boblannon reviewed Mar 9, 2018

View reviewed changes

boblannon approved these changes Mar 9, 2018

View reviewed changes

aarthykc reviewed Mar 13, 2018

View reviewed changes

Rename PhrasematchSubq as it appears in JS to PhrasematchSubqObject i…

c6d30e5

…n tests and docs, to avoid confusion with the C++ PhrasematchSubq struct type, which has slightly different properties

apendleton changed the base branch from cache-clean to master March 13, 2018 16:36

apendleton merged commit dfa468a into master Mar 13, 2018

apendleton deleted the cache-split branch March 13, 2018 17:05

This was referenced Mar 13, 2018

split into multiple files [wip] #58

Closed

Use -Werror and -isystem #118

Open

This was referenced Mar 13, 2018

Integer overflow in delta encoding in __get #79

Closed

Overflow in pxy2zxy #75

Closed

Conversation

apendleton commented Feb 16, 2018

Uh oh!

apendleton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apendleton Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boblannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

apendleton Mar 9, 2018 •

edited

Loading