parser: make parser single-pass #61

belochub · 2017-02-08T19:56:00Z

Eliminate jstp::parser::internal::PrepareString function thus avoiding extra memory copying.
Fix deleting comment blocks located inside of strings.
Fix key parsing not throwing error when it included spaces but was not enclosed in quotes.

Fixes: #60

aqrln · 2017-02-08T23:24:09Z

src/jsrs_parser.cc


  Type type;
-  if (!GetType(to_parse, to_parse + size, &type)) {
+
+  size_t start_pos = internal::SkipToNextToken(str, end);


To be technically correct, it should probably be ptrdiff_t, not size_t. Though they are the same things on x86/x86_64 and most other platforms.

Here it is not the case, because SkipToNextToken returns size_t, which is fine (there is no pointer subtraction to get result in this function), but generally you are right and there are other occurrences where it can be important. I guess, I will still review and check for any issues that may be caused by this.

I investigated this topic and found out that the main difference between size_t and ptrdiff_t is that former is unsigned integer, whereas latter is signed.
Also, there are a lot of arguments on this topic and generally it is recommended to use signed types for array indexing as well as pointer arithmetic.
At the same time Google C++ Style Guide says:

When appropriate, you are welcome to use standard types like size_t and ptrdiff_t.

We use int very often, for integers we know are not going to be too big, e.g., loop counters. Use plain old int for such things. You should assume that an int is at least 32 bits, but don't assume that it has more than 32 bits. If you need a 64-bit integer type, use int64_t or uint64_t.

I guess it will be better to substitute almost all size_t uses with ptrdiff_t or plain int, but we need to discuss which one will suit us better.
Also I think we need new issue and corresponding PR for this topic, so that we can talk it over and weigh all the pros and cons.

aqrln · 2017-02-08T23:26:01Z

src/jsrs_parser.cc

-  if (size != parsed_size) {
+      (kParseFunctions[type])(isolate, str + start_pos, end, &parsed_size);
+
+  parsed_size += internal::SkipToNextToken(str + start_pos + parsed_size, end);


Why not increment str pointer after each SkipToNextToken call? We don't need the initial address anyway.

For two reasons: it is const char* and to enable universal check on line 98.

aqrln · 2017-02-08T23:30:18Z

src/jsrs_parser.cc

+  bool is_single_line;
+
+  switch (str[1]) {
+    case '/': {


No need for braces.

I just followed Google C++ Styleguide.

Well, the Google C++ Styleguide states:

case blocks in switch statements can have curly braces or not, depending on your preference.

Then we need to decide whether we use them or don't.

I liked it more with braces, what is your opinion on this topic?

IDK, I would not use them and have never used them before in such cases, but I checked it out in my editor and this code snippet indeed seems to look prettier and more readable with braces.

Let's stick with braces then. We need to update it in other switch-statements, should I apply it in this pull request, or it is the case for another one?

Better raise a new PR.

aqrln · 2017-02-08T23:30:25Z

src/jsrs_parser.cc

    }
+    case '*': {


aqrln · 2017-02-08T23:31:00Z

src/jsrs_parser.cc

+      is_single_line = false;
+      break;
+    }
+    default: { // In case it is not a comment start


No need for braces.

Two spaces before a single-line comment.

aqrln · 2017-02-08T23:32:01Z

src/jsrs_parser.cc

-          result[j++] = str[i];
-        }
-      }
+  if (is_single_line) {


Consider using a ternary operator, though it's up to you.

aqrln · 2017-02-09T00:11:30Z

@belochub I don't feel well now so I will review the rest of the PR tomorrow.

belochub · 2017-02-10T01:28:09Z

Rebased on master and fixed indentation in single-line comment.

aqrln · 2017-02-10T09:27:07Z

@belochub mind changing Refs to Fixes in the commit metadata?

aqrln · 2017-02-10T09:31:11Z

src/jsrs_parser.cc

+          return String::Empty(isolate);
+        }
+        strncpy(result + res_index, symb, out_offset);
+        delete[] symb;


It would be nice to get rid of dynamic memory allocation here.

We can allocate a buffer on stack and pass it to GetControlChar by pointer.

Not related to this PR, though.

aqrln · 2017-02-10T09:39:01Z

src/jsrs_parser.cc

@@ -524,25 +545,21 @@ Local<String> ParseKeyInObject(Isolate*    isolate,
  } else {
    size_t current_length = 0;
    for (size_t i = 0; i < *size; i++) {
-      if (begin[i] == ':') {
+      if (begin[i] == '_' || (i != 0 ?


Maybe decouple the check into an inline function?

aqrln · 2017-02-10T09:41:38Z

src/jsrs_parser.cc

        if (current_length != 0) {
          result = String::NewFromUtf8(isolate, begin,
                                       NewStringType::kInternalized,
                                       static_cast<int>(current_length))
                                           .ToLocalChecked();
          break;
        } else {
-          THROW_EXCEPTION(SyntaxError, "Unexpected token :");
+          THROW_EXCEPTION(SyntaxError, "Unexpected identifier");
          return Local<String>();


I'd like it to be consistent. In some cases we use Local<String>(), in some cases — String::Empty(Isolate). Though it is the matter of a different PR since it was not introduced in this one.

aqrln · 2017-02-10T09:43:25Z

src/jsrs_parser.cc

        break;
      }
      current_key = ParseKeyInObject(isolate,
                                     begin + i,
                                     end,
                                     &current_length);
      i += current_length;
+      i += SkipToNextToken(begin + i, end);
+      if (begin[i] != ':') {
+        THROW_EXCEPTION(SyntaxError, "Unexpected token");


Maybe change the message to the more meaningful "Colon expected"?

aqrln · 2017-02-10T09:45:17Z

src/packet_parser.cc

  size_t parsed_size = 0;
-  const char* source = PrepareString(isolate, *in, in.length(), &total_size);
+  // const char* source = PrepareString(isolate, *in, in.length(), &total_size);


Commented-out line should be deleted.

aqrln · 2017-02-10T09:46:13Z

src/packet_parser.cc

@@ -29,9 +29,10 @@ namespace packet_parser {
 Local<String> ParseNetworkPackets(Isolate* isolate,
                                  const String::Utf8Value& in,
                                  Local<Array> out) {
-  size_t total_size = 0;
+  size_t total_size = in.length();


I think I can now be const size_t.

aqrln · 2017-02-10T09:51:39Z

LGTM if the commented line is removed. Other points are discussable.

* Eliminate `jstp::parser::internal::PrepareString` function thus avoiding extra memory copying. * Fix deleting comment blocks located inside of strings. * Fix key parsing not throwing error when it included spaces but was not enclosed in quotes. Fixes: #60

belochub · 2017-02-10T14:37:54Z

@aqrln updated to resolve some of the issues you addressed.

aqrln

LGTM

aqrln · 2017-02-10T14:47:08Z

@belochub other issues mentioned have to be extracted and posted on the issue tracker. Will you do it or I should take care of it?

belochub · 2017-02-10T14:52:15Z

@aqrln, I already have an idea for a new PR fixing some of the issues, so I guess I will also extract other issues as well.

* Eliminate `jstp::parser::internal::PrepareString` function thus avoiding extra memory copying. * Fix deleting comment blocks located inside of strings. * Fix key parsing not throwing error when it included spaces but was not enclosed in quotes. Fixes: #60 PR-URL: #61

belochub · 2017-02-10T14:56:55Z

Landed in 3b2c1d3.

* Eliminate `jstp::parser::internal::PrepareString` function thus avoiding extra memory copying. * Fix deleting comment blocks located inside of strings. * Fix key parsing not throwing error when it included spaces but was not enclosed in quotes. Fixes: #60 PR-URL: #61

This is a new and shiny first major release for `metarhia-jstp`. Changes include API refactoring and improvements, implementations of CLI, sessions, and application versions, native addon build optimizations, lots of bug fixes, test coverage increase, and other, less notable changes. This release also denotes the bump of the protocol version to v1.0. The only difference from the previous version of the protocol is that "old" heartbeat messages (`{}`) are now deprecated and `ping`/`pong` messages must be used for this purpose instead. Notable changes: * **src,build:** improve the native module subsystem *(Alexey Orlenko)* [#36](#36) **\[semver-minor\]** * **build:** compile in ISO C++11 mode *(Alexey Orlenko)* [#37](#37) **\[semver-minor\]** * **build:** improve error handling *(Alexey Orlenko)* [#40](#40) **\[semver-minor\]** * **lib:** refactor record-serialization.js *(Alexey Orlenko)* [#41](#41) * **parser:** fix a possible memory leak *(Alexey Orlenko)* [#44](#44) **\[semver-minor\]** * **protocol:** change the format of handshake packets *(Alexey Orlenko)* [#54](#54) **\[semver-major\]** * **parser:** make parser single-pass *(Mykola Bilochub)* [#61](#61) * **parser:** remove special case for '\0' literal *(Mykola Bilochub)* [#68](#68) **\[semver-major\]** * **parser:** fix bug causing node to crash *(Mykola Bilochub)* [#75](#75) * **client:** drop redundant callback argument *(Alexey Orlenko)* [#104](#104) **\[semver-major\]** * **client:** handle errors in connectAndInspect *(Alexey Orlenko)* [#105](#105) **\[semver-major\]** * **socket,ws:** use socket.destroy() properly *(Alexey Orlenko)* [#84](#84) **\[semver-major\]** * **cli:** add basic implementation *(Mykola Bilochub)* [#107](#107) **\[semver-minor\]** * **connection:** fix error handling in optional cbs *(Alexey Orlenko)* [#147](#147) **\[semver-major\]** * **test:** add JSON5 specs test suite *(Alexey Orlenko)* [#158](#158) * **lib:** change event signature *(Denys Otrishko)* [#187](#187) **\[semver-major\]** * **lib:** add address method to Server *(Denys Otrishko)* [#190](#190) **\[semver-minor\]** * **parser:** implement NaN and Infinity parsing *(Mykola Bilochub)* [#201](#201) * **parser:** improve string parsing performance *(Mykola Bilochub)* [#220](#220) * **lib:** optimize connection events *(Denys Otrishko)* [#222](#222) **\[semver-major\]** * **lib:** refactor server and client API *(Denys Otrishko)* [#209](#209) **\[semver-major\]** * **lib,src:** rename term packet usages to message *(Denys Otrishko)* [#270](#270) **\[semver-major\]** * **lib:** emit events about connection messages *(Denys Otrishko)* [#252](#252) **\[semver-minor\]** * **lib:** implement API versioning *(Denys Otrishko)* [#231](#231) **\[semver-minor\]** * **lib:** allow to set event handlers in application *(Denys Otrishko)* [#286](#286) **\[semver-minor\]** * **lib:** allow to broadcast events from server *(Denys Otrishko)* [#287](#287) **\[semver-minor\]** * **connection:** make callback method private *(Alexey Orlenko)* [#306](#306) **\[semver-major\]** * **lib:** implement sessions *(Mykola Bilochub)* [#289](#289) **\[semver-major\]** * **connection:** use ping-pong instead of heartbeat *(Dmytro Nechai)* [#303](#303) **\[semver-major\]**

* Eliminate `jstp::parser::internal::PrepareString` function thus avoiding extra memory copying. * Fix deleting comment blocks located inside of strings. * Fix key parsing not throwing error when it included spaces but was not enclosed in quotes. Fixes: metarhia/jstp#60 PR-URL: metarhia/jstp#61

belochub added bug optimization parser semver-patch labels Feb 8, 2017

belochub requested a review from aqrln February 8, 2017 19:56

aqrln reviewed Feb 8, 2017

View reviewed changes

src/jsrs_parser.cc

}

case '*': {

Copy link

Member

aqrln Feb 8, 2017

Choose a reason for hiding this comment

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

Ditto

aqrln reviewed Feb 8, 2017

View reviewed changes

belochub force-pushed the parser-single-pass branch from a95be15 to 1199fc3 Compare February 10, 2017 01:24

aqrln reviewed Feb 10, 2017

View reviewed changes

belochub force-pushed the parser-single-pass branch from 1199fc3 to aa8c617 Compare February 10, 2017 14:35

aqrln approved these changes Feb 10, 2017

View reviewed changes

belochub closed this Feb 10, 2017

belochub deleted the parser-single-pass branch February 10, 2017 14:56

aqrln mentioned this pull request Feb 20, 2017

v0.6.6 proposal #79

Merged

aqrln mentioned this pull request Apr 3, 2017

v0.5.2 proposal #118

Merged

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser: make parser single-pass #61

parser: make parser single-pass #61

belochub commented Feb 8, 2017 •

edited

Loading

aqrln Feb 8, 2017

belochub Feb 9, 2017

belochub Feb 9, 2017

aqrln Feb 8, 2017

belochub Feb 8, 2017

aqrln Feb 8, 2017

belochub Feb 8, 2017

aqrln Feb 8, 2017

belochub Feb 8, 2017

aqrln Feb 8, 2017

belochub Feb 8, 2017

aqrln Feb 8, 2017

aqrln Feb 8, 2017

aqrln Feb 8, 2017

aqrln Feb 8, 2017

belochub Feb 8, 2017

aqrln commented Feb 9, 2017

belochub commented Feb 10, 2017

aqrln commented Feb 10, 2017

aqrln Feb 10, 2017

aqrln Feb 10, 2017

aqrln Feb 10, 2017

aqrln Feb 10, 2017

aqrln Feb 10, 2017 •

edited

Loading

aqrln Feb 10, 2017

aqrln Feb 10, 2017

aqrln Feb 10, 2017

aqrln commented Feb 10, 2017 •

edited

Loading

belochub commented Feb 10, 2017

aqrln left a comment

aqrln commented Feb 10, 2017

belochub commented Feb 10, 2017

belochub commented Feb 10, 2017

parser: make parser single-pass #61

parser: make parser single-pass #61

Conversation

belochub commented Feb 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Feb 9, 2017

belochub commented Feb 10, 2017

aqrln commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aqrln commented Feb 10, 2017 • edited Loading

belochub commented Feb 10, 2017

aqrln left a comment

Choose a reason for hiding this comment

aqrln commented Feb 10, 2017

belochub commented Feb 10, 2017

belochub commented Feb 10, 2017

belochub commented Feb 8, 2017 •

edited

Loading

aqrln Feb 10, 2017 •

edited

Loading

aqrln commented Feb 10, 2017 •

edited

Loading