FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: general C++ cleanup in node_url.cc by addaleax · Pull Request #19598 · nodejs/node · GitHub

/ node Public

src: general C++ cleanup in node_url.cc - #19598

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:url-refactor
ConversationCommits (2)ChecksFiles changed
Closed

src: general C++ cleanup in node_url.cc#19598
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:url-refactor

Conversation

addaleax commented Mar 25, 2018

Copy link
Copy Markdown
Member

  • Merge domain and opaque storage in URL parser:

    This just simplifies the code a bit, having multiple fields
    in an union with the same type is usually just overhead.

  • Add move variant of URLHost::ToString():

    This helps avoid unnecessary string copy operations, especially
    since we control the lifetime of URLHost objects pretty well.

  • Use const refs in node_url.cc where appropriate

  • Remove or reduce overly generous .reserve() calls:

    These would otherwise keep a lot of unused memory lying around.

  • Return return values instead of unnecessary pointer arguments

  • Use more common/expressive variable names

  • Avoid macro use, reduce number of unnecessary JS strings:

    There’s no reason for GET, GET_AND_SET and UTF8STRING to be
    macros. Also, GET would previously create a JS string instance
    for each single call, even though the strings it was called
    with were compile-time constants.

  • Avoid unnecessary JS casts when the type of a value is known

  • Avoid (commonly unnecessary) copy for whitespace stripping

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

src: general C++ cleanup in node_url.cc
23b5f74 
- Merge `domain` and `opaque` storage in URL parser:

  This just simplifies the code a bit, having multiple fields
  in an union with the same type is usually just overhead.

- Add move variant of `URLHost::ToString()`:

  This helps avoid unnecessary string copy operations, especially
  since we control the lifetime of `URLHost` objects pretty well.

- Use const refs in node_url.cc where appropriate

- Remove or reduce overly generous `.reserve()` calls:

  These would otherwise keep a lot of unused memory lying around.

- Return return values instead of unnecessary pointer arguments

- Use more common/expressive variable names

- Avoid macro use, reduce number of unnecessary JS strings:

  There’s no reason for `GET`, `GET_AND_SET` and `UTF8STRING` to be
  macros. Also, `GET` would previously create a JS string instance
  for each single call, even though the strings it was called
  with were compile-time constants.

- Avoid unnecessary JS casts when the type of a value is known

- Avoid (commonly unnecessary) copy for whitespace stripping
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 25, 2018

addaleax commented Mar 25, 2018

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/17164/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/142/

TimothyGu approved these changes Mar 25, 2018
View reviewed changes

TimothyGu left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is awesome, thank you for this!

Just wondering, do the simplifications have any tangible effects on the benchmarks? Never mind, didn't see the triggered benchmark CI.

Comment thread src/node_url.cc
CHECK_EQ(type_, HostType::H_FAILED);
std::string output;
output.reserve(length * 3);
output.reserve(length);

TimothyGu Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why this change?

addaleax Mar 25, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@TimothyGu i’m not sure whether the spec definition says that this must be ASCII even before percent encoding, but either way, I would personally expect it to be ASCII most or all of the time?

TimothyGu Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fair enough.

jasnell Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As long as the tests still pass. :)

Comment thread src/node_url.cc Outdated
bool uflag = false;
bool atflag = false; // Set when @ has been seen.
bool square_bracket_flag = false; // Set inside of [...]
bool inside_password_flag = false; // Set after a : after an username.

TimothyGu Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

As a data point, the URL Standard calls it passwordTokenSeenFlag, but seems like you figured out what it meant on yourself :)

addaleax Mar 25, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@TimothyGu Idk, do you want me to change it? I think both of these are okay names?

TimothyGu Mar 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd slightly prefer the name in the spec for better correspondence for future readers.

addaleax commented Mar 25, 2018

Copy link
Copy Markdown
Member Author

Just wondering, do the simplifications have any tangible effects on the benchmarks?

Yes, and a pretty good one :) But I wanted to wait for the benchmark CI before I started putting numbers on this… ;)

addaleax added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 25, 2018

TimothyGu commented Mar 25, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Benchmark CI finished. The results are as follows (omitting some files with no or statistically insignificant results):

url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='auth'         ***     26.52 %       ±4.34%  ±5.82%  ±7.65%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='dot'          ***     27.04 %       ±4.29%  ±5.75%  ±7.57%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='file'         ***     16.13 %       ±5.29%  ±7.04%  ±9.17%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='idn'          ***     20.43 %       ±3.96%  ±5.29%  ±6.94%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='javascript'   ***     44.03 %       ±3.56%  ±4.76%  ±6.24%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='long'          **      7.95 %       ±4.59%  ±6.16%  ±8.12%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='percent'      ***     19.76 %       ±4.22%  ±5.65%  ±7.44%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='short'        ***     20.40 %       ±4.81%  ±6.40%  ±8.33%
url/legacy-vs-whatwg-url-parse.js n=100000 method='whatwg' type='ws'           ***     25.30 %       ±4.91%  ±6.54%  ±8.52%
url/whatwg-url-properties.js n=300000 prop='hash' input='auth'                 ***     42.18 %       ±7.66% ±10.26% ±13.48%
url/whatwg-url-properties.js n=300000 prop='hash' input='dot'                  ***     47.95 %       ±8.51% ±11.33% ±14.77%
url/whatwg-url-properties.js n=300000 prop='hash' input='file'                 ***     53.94 %       ±5.76%  ±7.67%  ±9.98%
url/whatwg-url-properties.js n=300000 prop='hash' input='idn'                  ***     48.08 %       ±8.31% ±11.08% ±14.49%
url/whatwg-url-properties.js n=300000 prop='hash' input='javascript'           ***     59.97 %       ±3.83%  ±5.13%  ±6.76%
url/whatwg-url-properties.js n=300000 prop='hash' input='long'                 ***     49.64 %       ±8.66% ±11.53% ±15.04%
url/whatwg-url-properties.js n=300000 prop='hash' input='percent'              ***     39.59 %       ±7.09%  ±9.45% ±12.33%
url/whatwg-url-properties.js n=300000 prop='hash' input='short'                ***     50.21 %       ±5.97%  ±7.95% ±10.36%
url/whatwg-url-properties.js n=300000 prop='hash' input='ws'                   ***     48.12 %       ±3.94%  ±5.25%  ±6.84%
url/whatwg-url-properties.js n=300000 prop='host' input='auth'                 ***     27.38 %       ±4.40%  ±5.87%  ±7.67%
url/whatwg-url-properties.js n=300000 prop='host' input='dot'                  ***     16.04 %       ±3.52%  ±4.69%  ±6.12%
url/whatwg-url-properties.js n=300000 prop='host' input='file'                 ***     29.68 %       ±6.20%  ±8.26% ±10.77%
url/whatwg-url-properties.js n=300000 prop='host' input='idn'                  ***     18.71 %       ±3.21%  ±4.27%  ±5.56%
url/whatwg-url-properties.js n=300000 prop='host' input='javascript'            **     -6.49 %       ±3.84%  ±5.12%  ±6.68%
url/whatwg-url-properties.js n=300000 prop='host' input='long'                 ***     19.49 %       ±2.52%  ±3.37%  ±4.40%
url/whatwg-url-properties.js n=300000 prop='host' input='percent'              ***     21.02 %       ±4.20%  ±5.63%  ±7.42%
url/whatwg-url-properties.js n=300000 prop='host' input='short'                ***     20.31 %       ±4.14%  ±5.51%  ±7.17%
url/whatwg-url-properties.js n=300000 prop='host' input='ws'                   ***     25.40 %       ±5.28%  ±7.03%  ±9.17%
url/whatwg-url-properties.js n=300000 prop='hostname' input='auth'             ***     33.21 %       ±8.57% ±11.48% ±15.10%
url/whatwg-url-properties.js n=300000 prop='hostname' input='dot'              ***     27.35 %       ±8.75% ±11.69% ±15.33%
url/whatwg-url-properties.js n=300000 prop='hostname' input='file'             ***     25.96 %       ±2.09%  ±2.78%  ±3.62%
url/whatwg-url-properties.js n=300000 prop='hostname' input='idn'              ***     25.93 %       ±7.78% ±10.38% ±13.58%
url/whatwg-url-properties.js n=300000 prop='hostname' input='javascript'        **     -5.25 %       ±3.70%  ±4.94%  ±6.48%
url/whatwg-url-properties.js n=300000 prop='hostname' input='long'             ***     24.51 %       ±7.95% ±10.60% ±13.85%
url/whatwg-url-properties.js n=300000 prop='hostname' input='percent'          ***     19.31 %       ±5.99%  ±7.99% ±10.41%
url/whatwg-url-properties.js n=300000 prop='hostname' input='short'            ***     31.71 %       ±7.15%  ±9.60% ±12.68%
url/whatwg-url-properties.js n=300000 prop='hostname' input='ws'               ***     24.53 %       ±2.99%  ±3.97%  ±5.17%
url/whatwg-url-properties.js n=300000 prop='href' input='auth'                 ***     12.44 %       ±3.83%  ±5.10%  ±6.64%
url/whatwg-url-properties.js n=300000 prop='href' input='dot'                  ***     16.99 %       ±5.63%  ±7.53%  ±9.89%
url/whatwg-url-properties.js n=300000 prop='href' input='file'                 ***     16.61 %       ±5.01%  ±6.67%  ±8.69%
url/whatwg-url-properties.js n=300000 prop='href' input='idn'                  ***     16.23 %       ±4.36%  ±5.80%  ±7.55%
url/whatwg-url-properties.js n=300000 prop='href' input='javascript'           ***     14.50 %       ±4.84%  ±6.44%  ±8.38%
url/whatwg-url-properties.js n=300000 prop='href' input='long'                 ***     22.06 %       ±4.47%  ±5.97%  ±7.82%
url/whatwg-url-properties.js n=300000 prop='href' input='percent'              ***     17.30 %       ±5.30%  ±7.05%  ±9.19%
url/whatwg-url-properties.js n=300000 prop='href' input='short'                ***     11.16 %       ±6.01%  ±8.01% ±10.44%
url/whatwg-url-properties.js n=300000 prop='href' input='ws'                   ***     15.35 %       ±3.44%  ±4.57%  ±5.95%
url/whatwg-url-properties.js n=300000 prop='origin' input='auth'                       -1.99 %       ±6.88%  ±9.16% ±11.95%
url/whatwg-url-properties.js n=300000 prop='origin' input='dot'                        -1.38 %       ±6.65%  ±8.85% ±11.53%
url/whatwg-url-properties.js n=300000 prop='origin' input='file'                       -7.85 %       ±8.77% ±11.69% ±15.26%
url/whatwg-url-properties.js n=300000 prop='origin' input='idn'                        -5.56 %       ±8.77% ±11.68% ±15.23%
url/whatwg-url-properties.js n=300000 prop='origin' input='javascript'                 -2.17 %       ±9.92% ±13.20% ±17.20%
url/whatwg-url-properties.js n=300000 prop='origin' input='long'                       -0.82 %       ±8.02% ±10.67% ±13.89%
url/whatwg-url-properties.js n=300000 prop='origin' input='percent'                    -6.63 %       ±7.59% ±10.14% ±13.28%
url/whatwg-url-properties.js n=300000 prop='origin' input='short'                      -2.48 %       ±8.38% ±11.16% ±14.54%
url/whatwg-url-properties.js n=300000 prop='origin' input='ws'                         -4.61 %       ±7.98% ±10.64% ±13.89%
url/whatwg-url-properties.js n=300000 prop='password' input='auth'                      0.93 %       ±5.27%  ±7.01%  ±9.14%
url/whatwg-url-properties.js n=300000 prop='password' input='dot'                       1.11 %       ±7.22%  ±9.61% ±12.51%
url/whatwg-url-properties.js n=300000 prop='password' input='file'              **     -5.54 %       ±4.02%  ±5.39%  ±7.08%
url/whatwg-url-properties.js n=300000 prop='password' input='idn'                       3.78 %       ±5.75%  ±7.65%  ±9.96%
url/whatwg-url-properties.js n=300000 prop='password' input='javascript'               -1.67 %       ±3.69%  ±4.93%  ±6.45%
url/whatwg-url-properties.js n=300000 prop='password' input='long'               *      6.24 %       ±5.95%  ±7.93% ±10.35%
url/whatwg-url-properties.js n=300000 prop='password' input='percent'                   2.14 %       ±6.21%  ±8.27% ±10.77%
url/whatwg-url-properties.js n=300000 prop='password' input='short'                     2.53 %       ±5.29%  ±7.04%  ±9.17%
url/whatwg-url-properties.js n=300000 prop='password' input='ws'                        3.04 %       ±6.41%  ±8.53% ±11.12%
url/whatwg-url-properties.js n=300000 prop='pathname' input='auth'             ***     26.96 %       ±6.75%  ±8.98% ±11.69%
url/whatwg-url-properties.js n=300000 prop='pathname' input='dot'              ***     26.50 %       ±4.73%  ±6.30%  ±8.22%
url/whatwg-url-properties.js n=300000 prop='pathname' input='file'             ***     17.49 %       ±4.38%  ±5.84%  ±7.63%
url/whatwg-url-properties.js n=300000 prop='pathname' input='idn'              ***     24.41 %       ±7.95% ±10.60% ±13.83%
url/whatwg-url-properties.js n=300000 prop='pathname' input='javascript'                1.09 %       ±6.29%  ±8.39% ±10.96%
url/whatwg-url-properties.js n=300000 prop='pathname' input='long'             ***     23.58 %       ±3.92%  ±5.22%  ±6.81%
url/whatwg-url-properties.js n=300000 prop='pathname' input='percent'          ***     23.47 %       ±5.90%  ±7.86% ±10.23%
url/whatwg-url-properties.js n=300000 prop='pathname' input='short'            ***     23.85 %       ±7.76% ±10.35% ±13.50%
url/whatwg-url-properties.js n=300000 prop='pathname' input='ws'               ***     20.85 %       ±6.07%  ±8.08% ±10.52%
url/whatwg-url-properties.js n=300000 prop='port' input='auth'                 ***     27.75 %       ±4.52%  ±6.04%  ±7.90%
url/whatwg-url-properties.js n=300000 prop='port' input='dot'                  ***     23.87 %       ±9.07% ±12.10% ±15.81%
url/whatwg-url-properties.js n=300000 prop='port' input='file'                   *     -8.14 %       ±7.66% ±10.23% ±13.41%
url/whatwg-url-properties.js n=300000 prop='port' input='idn'                  ***     30.71 %       ±8.05% ±10.82% ±14.32%
url/whatwg-url-properties.js n=300000 prop='port' input='javascript'                   -3.64 %       ±4.64%  ±6.18%  ±8.04%
url/whatwg-url-properties.js n=300000 prop='port' input='long'                 ***     16.97 %       ±5.39%  ±7.21%  ±9.45%
url/whatwg-url-properties.js n=300000 prop='port' input='percent'              ***     21.40 %       ±6.49%  ±8.66% ±11.30%
url/whatwg-url-properties.js n=300000 prop='port' input='short'                ***     25.71 %       ±6.74%  ±9.03% ±11.89%
url/whatwg-url-properties.js n=300000 prop='port' input='ws'                   ***     22.03 %       ±9.58% ±12.80% ±16.75%
url/whatwg-url-properties.js n=300000 prop='protocol' input='auth'             ***     44.08 %       ±8.89% ±11.93% ±15.74%
url/whatwg-url-properties.js n=300000 prop='protocol' input='dot'              ***     47.91 %       ±5.36%  ±7.18%  ±9.45%
url/whatwg-url-properties.js n=300000 prop='protocol' input='file'                      1.36 %       ±4.43%  ±5.94%  ±7.81%
url/whatwg-url-properties.js n=300000 prop='protocol' input='idn'              ***     37.39 %       ±3.56%  ±4.76%  ±6.24%
url/whatwg-url-properties.js n=300000 prop='protocol' input='javascript'       ***     64.91 %       ±5.15%  ±6.91%  ±9.10%
url/whatwg-url-properties.js n=300000 prop='protocol' input='long'             ***     41.68 %       ±6.53%  ±8.69% ±11.32%
url/whatwg-url-properties.js n=300000 prop='protocol' input='percent'          ***     38.31 %       ±6.77%  ±9.01% ±11.73%
url/whatwg-url-properties.js n=300000 prop='protocol' input='short'            ***     49.92 %       ±9.32% ±12.55% ±16.63%
url/whatwg-url-properties.js n=300000 prop='protocol' input='ws'               ***     42.48 %       ±6.20%  ±8.27% ±10.79%
url/whatwg-url-properties.js n=300000 prop='search' input='auth'               ***     48.76 %      ±11.10% ±14.90% ±19.67%
url/whatwg-url-properties.js n=300000 prop='search' input='dot'                ***     40.16 %       ±9.03% ±12.07% ±15.81%
url/whatwg-url-properties.js n=300000 prop='search' input='file'               ***     46.96 %       ±7.02%  ±9.43% ±12.45%
url/whatwg-url-properties.js n=300000 prop='search' input='idn'                ***     35.46 %       ±6.78%  ±9.03% ±11.77%
url/whatwg-url-properties.js n=300000 prop='search' input='javascript'         ***     44.57 %       ±7.22%  ±9.61% ±12.52%
url/whatwg-url-properties.js n=300000 prop='search' input='long'               ***     38.18 %       ±5.91%  ±7.86% ±10.24%
url/whatwg-url-properties.js n=300000 prop='search' input='percent'            ***     39.50 %       ±5.79%  ±7.70% ±10.03%
url/whatwg-url-properties.js n=300000 prop='search' input='short'              ***     39.02 %       ±5.87%  ±7.85% ±10.30%
url/whatwg-url-properties.js n=300000 prop='search' input='ws'                 ***     41.64 %      ±10.63% ±14.16% ±18.48%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='auth'                 12.89 %      ±13.72% ±18.29% ±23.86%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='dot'                  -5.73 %      ±11.66% ±15.53% ±20.23%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='file'                 -9.78 %      ±13.29% ±17.71% ±23.09%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='idn'                  -5.48 %      ±10.98% ±14.62% ±19.05%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='javascript'            5.91 %      ±13.99% ±18.62% ±24.25%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='long'                  6.83 %      ±11.86% ±15.80% ±20.61%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='percent'               1.14 %      ±13.74% ±18.28% ±23.80%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='short'                -2.59 %      ±14.64% ±19.50% ±25.42%
url/whatwg-url-properties.js n=300000 prop='searchParams' input='ws'                    6.80 %      ±14.36% ±19.11% ±24.88%
url/whatwg-url-properties.js n=300000 prop='username' input='auth'                      6.73 %       ±6.89%  ±9.18% ±11.96%
url/whatwg-url-properties.js n=300000 prop='username' input='dot'                      -0.61 %       ±7.45%  ±9.92% ±12.91%
url/whatwg-url-properties.js n=300000 prop='username' input='file'                     -1.45 %       ±6.47%  ±8.63% ±11.26%
url/whatwg-url-properties.js n=300000 prop='username' input='idn'                      -4.26 %       ±5.94%  ±7.90% ±10.28%
url/whatwg-url-properties.js n=300000 prop='username' input='javascript'        **     -5.42 %       ±3.98%  ±5.33%  ±6.99%
url/whatwg-url-properties.js n=300000 prop='username' input='long'                     -1.31 %       ±7.04%  ±9.36% ±12.19%
url/whatwg-url-properties.js n=300000 prop='username' input='percent'                   0.62 %       ±7.86% ±10.46% ±13.62%
url/whatwg-url-properties.js n=300000 prop='username' input='short'                     0.53 %       ±6.01%  ±8.01% ±10.45%
url/whatwg-url-properties.js n=300000 prop='username' input='ws'                       -4.61 %       ±6.80%  ±9.05% ±11.78%

That's some quite substantial speedups!

  • parsing: 8-44%
  • property setters: up to 60% (!)

The only performance decreases look fluky.

TimothyGu commented Mar 25, 2018

Copy link
Copy Markdown
Member

Comparison for url/legacy-vs-whatwg-url-parse.js:

Before:

After:

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 29, 2018

addaleax commented Mar 30, 2018

Copy link
Copy Markdown
Member Author

Landed in ae70e2b

addaleax closed this Mar 30, 2018
addaleax deleted the url-refactor branch March 30, 2018 12:36
addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 30, 2018
addaleax added a commit that referenced this pull request Mar 30, 2018
src: general C++ cleanup in node_url.cc
ae70e2b 
- Merge `domain` and `opaque` storage in URL parser:

  This just simplifies the code a bit, having multiple fields
  in an union with the same type is usually just overhead.

- Add move variant of `URLHost::ToString()`:

  This helps avoid unnecessary string copy operations, especially
  since we control the lifetime of `URLHost` objects pretty well.

- Use const refs in node_url.cc where appropriate

- Remove or reduce overly generous `.reserve()` calls:

  These would otherwise keep a lot of unused memory lying around.

- Return return values instead of unnecessary pointer arguments

- Use more common/expressive variable names

- Avoid macro use, reduce number of unnecessary JS strings:

  There’s no reason for `GET`, `GET_AND_SET` and `UTF8STRING` to be
  macros. Also, `GET` would previously create a JS string instance
  for each single call, even though the strings it was called
  with were compile-time constants.

- Avoid unnecessary JS casts when the type of a value is known

- Avoid (commonly unnecessary) copy for whitespace stripping

PR-URL: #19598
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this pull request Apr 2, 2018
src: general C++ cleanup in node_url.cc
01e3190 
- Merge `domain` and `opaque` storage in URL parser:

  This just simplifies the code a bit, having multiple fields
  in an union with the same type is usually just overhead.

- Add move variant of `URLHost::ToString()`:

  This helps avoid unnecessary string copy operations, especially
  since we control the lifetime of `URLHost` objects pretty well.

- Use const refs in node_url.cc where appropriate

- Remove or reduce overly generous `.reserve()` calls:

  These would otherwise keep a lot of unused memory lying around.

- Return return values instead of unnecessary pointer arguments

- Use more common/expressive variable names

- Avoid macro use, reduce number of unnecessary JS strings:

  There’s no reason for `GET`, `GET_AND_SET` and `UTF8STRING` to be
  macros. Also, `GET` would previously create a JS string instance
  for each single call, even though the strings it was called
  with were compile-time constants.

- Avoid unnecessary JS casts when the type of a value is known

- Avoid (commonly unnecessary) copy for whitespace stripping

PR-URL: #19598
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos mentioned this pull request Apr 4, 2018
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
danbev danbev approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL