performance enhancements by jdalton · Pull Request #47 · nirgit/simple-xml-to-json

jdalton · 2024-04-25T04:56:25Z

I've optimized the lib:

Before for 4,000 iterations:

avg exec time of 4000 iterations (in ms): 1.7665
...
Time: 7.21 s

After for 4,000 iterations:

avg exec time of 4000 iterations (in ms): 0.438
...
Time: 1.954 s, estimated 2 s

nirgit · 2024-04-26T12:55:37Z

thanks for the PR @jdalton , i'll be sure to go over it soon

jdalton · 2024-04-28T20:37:17Z

@nirgit I updated it to keep the source readable and inline during build. I've added a way to annotate what is inlined. It allows for fine grain control and seems to be working well.

nirgit · 2024-04-30T04:31:01Z

@nirgit I updated it to keep the source readable and inline during build. I've added a way to annotate what is inlined. It allows for fine grain control and seems to be working well.

thanks @jdalton for the update, and for the contribution of this PR, very much appreciated.
i see that the amount of changes is getting quite big, which makes it longer/harder to review. so i suggest to perhaps stop at this point and allow a review and feedback take place.
in case you have more changes planned, perhaps you could introduce those later on a following PR.

i'll try to go over the PR in the upcoming week.
thanks again

jdalton · 2024-04-30T21:22:14Z

@nirgit I split this PR out into ~~#48~~ and ~~#49~~.
Once those are merged I'll update this and convert it from DRAFT to READY for REVIEW.

nirgit

big PR.
i'd rather having this PR separated into several smaller PRs so its easier to review.
for instance:

PR for converting everything to constants
PR for refactoring the lexer
PR for refactoring the parser

that way it'd be a whole lot easier to track, sorry.

i did not finish reviewing everything, and still need to go over the parser.js part and the astToJson.js, but since there are already some comments, i thought i'd already send those in so you can have a look, and we'll take it from there.

thanks

nirgit · 2024-05-14T13:58:20Z


 const { readXMLFile } = require('./testUtils')
-const { convertXML } = require('../src/xmlToJson')
+const { convertXML } = require('../lib/simpleXmlToJson.min.js')


cool, but this means that running npm test cannot happen without running npm run build. pls fix

I can make it build before tests are run :)

nirgit · 2024-05-14T13:59:29Z

+        const iterations = 4000
+        for (let i = 0; i < iterations; i += 1) {


nirgit · 2024-05-14T14:01:03Z

why all this hassle? why not just rename transpiler.js to parser.js :)?

nirgit · 2024-05-14T14:02:41Z

any changes here?
if there aren't any, lets just do a rename for transpiler.js to parser.js?

nirgit · 2024-05-14T14:03:00Z

-const Token = (type, value) => ({
-    type,
-    value
+const Token = ($type, $value = '') => ({


Inlining vars is easier when they aren't shortcut properties.

nirgit · 2024-05-18T18:59:07Z

+    let peekedPos = 0
+    let peekedTagName = ''


unused. remove?

nirgit · 2024-05-18T19:00:48Z

-const isCharBlank = (char) =>
-    char === ' ' || char === '\n' || char === '\r' || char === '\t'
+function createLexer(xmlAsString, { knownAttrib, knownElement } = {}) {
+    const { length } = xmlAsString


pls check if xmlAsString is null or undefined

nirgit · 2024-05-18T19:08:45Z

+    const scoping = []
+    let currScope = 0
+    let currToken = EOF_TOKEN
+    let currTokenType = TOKEN_TYPE.EOF


why do we need this if we have currToken? we can just check currToken.type

We will for another biggie optimization after (accepting known elements and known attributes and skipping object creation and string slicing for those skipped).

nirgit · 2024-05-18T19:10:50Z

+                (code >= CHAR_CODE.DIGIT_0 && code <= CHAR_CODE.DIGIT_9) ||
+                code === CHAR_CODE.LODASH ||
+                code === CHAR_CODE.COLON ||
+                code === CHAR_CODE.HYPHEN


i think period should also be included
|| code === CHAR_CODE.PERIOD

tag names can only start with a letter or an underscore.
so its a problematic check to include numbers and other invalid chars in the same if.

Good to know. Currently (master branch) does allow digits and other chars:

simple-xml-to-json/src/lexer.js

Line 93 in ec2e488

const ELEMENT_TYPE_MATCHER = /[a-zA-Z0-9_:-]/

The inlining of the regexp reflects what is in master (more or less).

@nirgit

i think period should also be included

Included in an Element tagName ?

nirgit · 2024-05-18T19:57:06Z

+                                    : Token(
+                                        /*inline*/
+                                        TOKEN_TYPE.CONTENT,
+                                        buffer +
+                                                readAlphaNumericAndSpecialChars()
+                                    )
+                            return currToken


maybe i'm missing it, but this does not seem equivalent to the old implementation reading special chars.
i think you might not be reading the suffix appropriately, maybe i'm wrong.
can you pls double check?

The 'should support textual content for elements', test passes on the jdalton/optimize branch.

jdalton force-pushed the jdalton/optimize branch 2 times, most recently from 30a727c to 5170b03 Compare April 25, 2024 12:24

jdalton force-pushed the jdalton/optimize branch 5 times, most recently from 67ed86f to acf1b4d Compare April 28, 2024 20:36

jdalton force-pushed the jdalton/optimize branch 9 times, most recently from a6aef3c to 3be9669 Compare April 29, 2024 07:10

jdalton marked this pull request as draft April 30, 2024 21:19

jdalton force-pushed the jdalton/optimize branch 9 times, most recently from e31a822 to c8cd0d4 Compare May 3, 2024 20:22

jdalton mentioned this pull request May 3, 2024

add support for inlining methods #49

Merged

jdalton force-pushed the jdalton/optimize branch 10 times, most recently from 0fd1c66 to 062161b Compare May 10, 2024 14:10

jdalton marked this pull request as ready for review May 10, 2024 14:11

jdalton force-pushed the jdalton/optimize branch 6 times, most recently from bd2488e to 0cc4a79 Compare May 13, 2024 18:58

performance enhancements

68d9dfa

jdalton force-pushed the jdalton/optimize branch from 0cc4a79 to 68d9dfa Compare May 13, 2024 19:05

nirgit reviewed May 18, 2024

View reviewed changes

		const iterations = 4000
		for (let i = 0; i < iterations; i += 1) {

Uh oh!

Conversation

jdalton commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirgit commented Apr 26, 2024

Uh oh!

jdalton commented Apr 28, 2024

Uh oh!

nirgit commented Apr 30, 2024

Uh oh!

jdalton commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirgit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdalton May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jdalton commented Apr 25, 2024 •

edited

Loading

jdalton commented Apr 30, 2024 •

edited

Loading

jdalton May 20, 2024 •

edited

Loading