Refactor Plan about carmen-cache HOT 9 CLOSED

any gotchas we should be aware of either about switching those to 6, or dropping 4 more generally?

No gochas, feel free to move the build. Should work fine. Also, would appreciate a PR at https://github.com/mapbox/node-cpp-skel doing the same thing (since we should move to v6 as default there given the EOL of 2018-04-30 mentioned at https://github.com/nodejs/Release#release-schedule)

from carmen-cache.

👍 to this plan

from carmen-cache.

Finished divvying up functions into individual files, and associated headers. The only class that is not represented in a header is the UInt32Comparator which is created in normalizationcache.cpp. We chose to leave this off since it is only used in the normalization cache, and was fairly redundant to have in the normalizationcache.hpp file as well.

from carmen-cache.

This week we completed the work of splitting up the codebase into more manageable pieces, and have started working on docs, which we hope to have done early next week. We've also decided not to pursue a more direct adaptation of the benchmark work from core-tech, as it seems mostly to be geared towards runtime configurability for which we don't have any real usecase -- we do already benchmark have benchmarks in place for our performance-sensitive code, following the X.bench.js naming pattern in our test directory.

As such, the main remaining items after docs will be some Travis config tweaks (again per core-tech's recommendation), and a reorganization of carmen-cache's tests. We expect to be able to complete these by the end of next week.

Once this is all done and after team review we'll want to cut a release that we can push upstream to carmen, etc.. We'll determine timing on upstream PRs in coordination with the respective refactor teams.

from carmen-cache.

@springmeyer quick question when you get a sec: we're standardizing geocoding-team-wide on node 6+, so we were thinking about dropping node 4 from our build matrix. The ASAN/debug build and the GCC build only run on 4 at the moment; any gotchas we should be aware of either about switching those to 6, or dropping 4 more generally?

from carmen-cache.

@springmeyer moving the conversation about bench tweaks back here just for geocoding team discoverability and to not muddy up the process ticket, but just to make it concrete, here's what I'm thinking as a first pass at benchmark tweaks:

• move test/*.bench.test.js (of which I guess there's just one right now) into a new /bench directory
• adjust the test runners to still run the benches in this new directory to ensure that they don't break
• remove timing assertions
• remove skipping of bench or changes of bench behavior for particular test modes (DEBUG, ASAN)
• add a note to CONTRIBUTING.md recommending that contributors manually run benchmarks to compare a new branch and master to avoid performance regressions

I think, just given the wider team's timeboxing of the respective refactor groups' projects to the end of this week, we'd hold off on either writing additional benchmarks of more functionality, or significantly augmenting the current benchmarks with more robust/representative test data, but would ticket those out as things to come back to in a subsequent pass.

Does that sound good to you?

from carmen-cache.

Great plan @apendleton - sounds good 👍

from carmen-cache.

@boblannon and @aarthykc we're done with our checklist and are passing tests both here and in carmen so #116 should be good for your review.

A couple notes:

• C++ functions callable from Node have a different signature in C++ terms than in JS terms: from C++, they take one argument (called info) and return nothing; they access the JS args and set the JS return value by calling methods on info. We used JSDoc, but decided to stick with JS conventions, so, params and return type are as they appear from JS, and functions not directly callable from JS don't have JSDoc declarations. For most internal functions, though, we did add non-JSDoc-formatted comments explaining what each does.
• Our refactor separated out common utility functions into two different files, one for utilities that depend on the Node ecosystem and one for pure-C++ functions that don't. The rationale there was to eventually make it possible to unit test the latter flavor directly in C++ rather than indirectly via integration tests called from JS. There are almost no functions that actually fit this criterion right now, though, and the ones we have have pretty trivial functionality, so it wasn't worth bolting on a second unit-testing framework just yet. What we've done so far is, I guess, the start of that reorganization, and we'll think about factoring more pure-C++ logic out of the Node-flavored code, and testing it explicitly, as part of future work.

from carmen-cache.

This is done in #116

from carmen-cache.