Skip to content

[WIP] lib: add initial mapproxy implementation and benchmark - #7972

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:mapproxy
Closed

jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:mapproxy

Conversation

@jasnell

@jasnell jasnell commented Aug 4, 2016 •

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

internal

Description of change

Discussion around #6102 has explored the
possibility that a Map based implementation may be better, but there are
backwards compatibility concerns over changing the API. This commit adds
and internal implementation of a Map proxy object that allows a Map instance
to act like it is an ordinary dictionary object as much as possible.

A benchmark is included to show the perf differences.

This is a work in progress experiment that should not yet be landed.

/cc @nodejs/ctc (@mscdex @ofrobots in particular)

Discussion around nodejs#6102 has explored the
possibility that a Map based implementation may be better, but there are
backwards compatibility concerns over changing the API. This commit adds
and internal implementation of a Map proxy object that allows a Map instance
to act like it is an ordinary dictionary object as much as possible.

A benchmark is included to show the perf differences.

This is a work in progress experiment that should not yet be landed.
@jasnell jasnell added wip Issues and PRs that are still a work in progress. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Aug 4, 2016
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. mapproxy labels Aug 4, 2016
@mscdex mscdex removed the build Issues and PRs related to Node.js builds or CI infrastructure. label Aug 4, 2016
@ofrobots

ofrobots commented Aug 4, 2016

Copy link
Copy Markdown
Contributor

I suspect this is going to be slow because access to proxied properties is slow. I was thinking something like this instead:

function wrap(map) {
  map.prototype = new Proxy(map, handler);
  return map;
}

This way the map can be accessed as an object OR as an Map. The idea would be to encourage people to use the as-Map API and deprecate the as-Object API.

@jasnell

jasnell commented Aug 4, 2016

Copy link
Copy Markdown
Member Author

Yep, it's pretty slow in comparison...

$ ./node --expose-internals benchmark/misc/mapproxy.js
100000
misc/mapproxy.js m="nullproto" n=100000: 2048197.528825103
100000
misc/mapproxy.js m="map" n=100000: 3982209.2412820184
100000
misc/mapproxy.js m="mapproxy" n=100000: 373364.24271531263

Mixing the two like you suggest might be problematic. For instance, what happens if there is a header named "get" or "set". I'll play around with it but I'm not quite as optimistic.

@mscdex

mscdex commented Aug 4, 2016

Copy link
Copy Markdown
Contributor

Yes that was one of my concerns also, having "get" or "set" header names.

@mscdex mscdex removed the mapproxy label Aug 4, 2016
@jasnell

jasnell commented Aug 4, 2016

Copy link
Copy Markdown
Member Author

(aside... I find it quite funny that the bot created a new label for something that doesn't actually exist officially yet... may want to fix that ... /cc @Fishrock123 )

@jasnell jasnell changed the title lib: add initial mapproxy implementation and benchmark [WIP] lib: add initial mapproxy implementation and benchmark Aug 16, 2016
@jasnell jasnell closed this Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs involving general changes in the lib/ or src/ directories. wip Issues and PRs that are still a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants