Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code And Learn 2018 #1

Closed
50 of 62 tasks
hiroppy opened this issue Nov 24, 2018 · 3 comments
Closed
50 of 62 tasks

Code And Learn 2018 #1

hiroppy opened this issue Nov 24, 2018 · 3 comments

Comments

@hiroppy
Copy link
Member

hiroppy commented Nov 24, 2018

[lib]: Convert to Arrow Function

  • lib/_http_client.js
  • lib/_http_common.js
  • lib/_http_outgoing.js
  • lib/_stream_readable.js
  • lib/_tls_common.js
  • lib/fs.js
  • lib/internal/bootstrap_node.js
  • lib/internal/child_process.js
  • lib/internal/crypto/cipher.js
  • lib/internal/process.js
  • lib/internal/process/warning.js
  • lib/internal/repl.js:148
  • lib/internal/socket_list.js
  • lib/internal/streams/async_iterator.js
  • lib/internal/v8_prof_processor.js
  • lib/net.js
  • lib/punycode.js
  • lib/readline.js
  • lib/repl.js
  • lib/zlib.js

[src]: deprecated V8 API migration (ref)

  • src/bootstrapper.cc
  • src/node_serdes.cc
  • src/node_util.cc
  • src/string_decoder.cc
  • src/timer_wrap.cc
  • src/uv.cc

[test]: assert.strictEqual (ref)

  • test/parallel/test-child-process-cwd.js
  • test/parallel/test-buffer-alloc.js
  • test/parallel/test-crypto-fips.js
  • test/parallel/test-crypto-lazy-transform-writable.js
  • test/parallel/test-event-emitter-remove-all-listeners.js
  • test/parallel/test-file-write-stream.js
  • test/parallel/test-fs-write-buffer.js
  • test/parallel/test-fs-write-string-coerce.js
  • test/parallel/test-fs-write.js
  • test/parallel/test-http-1.0.js
  • test/parallel/test-http-default-encoding.js
  • test/parallel/test-http-parser.js
  • test/parallel/test-http-pause.js
  • test/parallel/test-http-request-end.js
  • test/parallel/test-http-upgrade-agent.js
  • test/parallel/test-http-upgrade-client.js
  • test/parallel/test-http.js
  • test/parallel/test-http2-binding.js
  • test/parallel/test-http2-multiheaders-raw.js
  • test/parallel/test-readline.js
  • test/parallel/test-repl-envvars.js
  • test/parallel/test-repl-persistent-history.js
  • test/parallel/test-repl-underscore.js
  • test/parallel/test-stream-readable-destroy.js
  • test/parallel/test-stream-writable-destroy.js
  • test/parallel/test-stream-writev.js
  • test/parallel/test-stream2-basic.js
  • test/parallel/test-util-deprecate.js
  • test/pummel/test-http-many-keep-alive-connections.js

[docs]: replace anonymous function with arrow function (ref)

  • doc/api/async_hooks.md
  • doc/api/errors.md
  • doc/api/util.md
  • doc/api/v8.md
  • doc/guides/writing-and-running-benchmarks.md
  • doc/guides/writing-tests.md

[docs]: typo

  • Typo "this this" doc/api/http2.md:973

Others

Let’s rename all error arguments in documentation to err

@joyeecheung
Copy link

joyeecheung commented Nov 24, 2018

About the C++ tasks (src files)

Introduction

Previously v8 only has C++ APIs to

  1. Create a JS Array via Array::New(size)
  2. Set the elements of a JS array via Object::Set()

So for example to create a JS array of length 2 with known elements
in the C++ land, you'd usually do:

Local<Array> arr = Array::New(isolate);
arr->Set(context, 0, some_value).FromJust();
arr->Set(context, 1, another_value).FromJust();

This is conceptually equivalent to JS code:

const arr = [];
arr[0] = some_value;
arr[1] = another_value;

Now there is a new API added in v8 that allows you to convert a C
array of Local<Value> with known length into a v8::Array easily:

Local<Value> values[] = { some_value, another_value };
// arraysize is a convenience template defined in node_internals.h
Local<Array> arr = Array::New(isolate, values, arraysize(values));

This can also be used to convert the values in a contiguous container e.g.
a std::vector

std::vector<Local<Value>> values;
// ....some code pushes a bunch of Local<Value> into the vector
Local<Array> arr = Array::New(isolate, values.data(), values.size());

The task is:

  1. Find patterns that uses the old APIs (for example, search for Array::New that
    only passes isolate and a size, or just isolate and the default size 0), and
    check if it can be converted to use the
    new API. That is:
    • The elements being added is already in a contiguous container like std::vector
    • ..or, they are in a C array
    • ..or, if they are just a few known values that can be initialized in a C array
      on the stack
  2. And covert the code to use the new API. It should be faster because the pattern
    array->Set() essentially goes through a very generic Object::Set which is slow,
    whereas the new API just packs the elements in a backing store in one go. And
    the usage should be easier to read than the old pattern.

Example

For more background, see nodejs#24125

diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc
index c40b855b99..93c1035617 100644
--- a/src/bootstrapper.cc
+++ b/src/bootstrapper.cc
@@ -50,11 +50,12 @@ void SetupNextTick(const FunctionCallbackInfo<Value>& args) {
           .ToLocalChecked();
   run_microtasks_fn->SetName(FIXED_ONE_BYTE_STRING(isolate, "runMicrotasks"));

-  Local<Array> ret = Array::New(isolate, 2);
-  ret->Set(context, 0, env->tick_info()->fields().GetJSArray()).FromJust();
-  ret->Set(context, 1, run_microtasks_fn).FromJust();
+  Local<Value> ret[] = {
+    env->tick_info()->fields().GetJSArray(),
+    run_microtasks_fn
+  };

-  args.GetReturnValue().Set(ret);
+  args.GetReturnValue().Set(Array::New(isolate, ret, arraysize(ret)));
 }

 void PromiseRejectCallback(PromiseRejectMessage message) {

@sotayamashita
Copy link
Member

sotayamashita commented Nov 24, 2018

List of solved issues:

  • lib/_http_outgoing.js
  • [test]: test/pummel/test-http-many-keep-alive-connections.js
  • [test]: test/parallel/test-child-process-cwd.js
  • doc/api/util.md

@ikasumiwt
Copy link

src/timer_wrap.cc file is removed by https://github.com/nodejs/node/pull/21777/files PR

Trott pushed a commit to Trott/io.js that referenced this issue Nov 24, 2018
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: nodejs#24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit to nodejs/node that referenced this issue Nov 25, 2018
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: #24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@hiroppy hiroppy closed this as completed Nov 26, 2018
rvagg pushed a commit to nodejs/node that referenced this issue Nov 28, 2018
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: #24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: nodejs#24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this issue Feb 11, 2019
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: #24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
rvagg pushed a commit to nodejs/node that referenced this issue Feb 28, 2019
Switched the order of arguments for strictEqual checks inside of
test/paralell/test-http2-binding.js

PR-URL: #24616
Refs: nodejsjp#1
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants