add support for inlining methods

[jdalton]

Add support for inlining methods.

Intended after #48.

[nirgit]

thanks @jdalton

could you give some detailed explanation on the work done on the rollup.config.js part?
i don't understand what you are trying to achieve and if the code complexity tradeoff is worth it
(assuming this is done for perf optimization, it'd be helpful to see some test measurements of "before" vs "after")

[nirgit]

explain?

[jdalton]

Languages like Zig and others have a notion of at "compile time". So when we are building the lib output with rollup and we load the modules it is at "compile time" this way during compile time we can export things that need to be inlined else not export them. The rollup "replace" plugin takes care of it seamlessly.

[jdalton]

I moved the "inline" stuff to its own module. It's designed in a way that you can not inline and all the code still works. Inlining just allows code to be annotated like peek(/*inline*/) and it will inline the code at compile time but keep it abstracted during development.

You can see this here:

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

Lines 94 to 99 in 6422842

	if (code === CHAR_CODE.OPEN_BRACKET) {
	pos += 1
	if (
	hasNext(/*inline*/) &&
	peek(/*inline*/) === CHAR_CODE.FORWARD_SLASH
	) {

when inlined it will become:

if (code === 60) {
  pos += 1
  if (
      (currentToken !== EOF_TOKEN && pos < length) &&
      (xmlAsString.charCodeAt(pos)) === 47
  ) {

[jdalton]

JavaScript engines perform inlining during their JIT phase. However because parsing/tokenizing/creating ast can be intensive having it inlined upfront can aid JS engines in their JIT because more things are known up-front (less calling out to other functions).

[jdalton]

With inlining enabled in PR #47 the performance is:

After for 4,000 iterations:

avg exec time of 4000 iterations (in ms): 0.553
...
Time: 2.459 s, estimated 3 s

without inlining (so removing --inline from the run command) it is:

After for 4,000 iterations:

avg exec time of 4000 iterations (in ms): 0.743
...
Time: 3.206 s

The idea is that inlining is bonus and so that's why it's an easy on/off and code works without inlining. The inlining bit is an extra oomph that does produce consistently faster results.

[jdalton]

Updated to simplify the inline bits by using acorn to reduce reliance on regexp.

[nirgit]

cool stuff @jdalton sorry for the delay reviewing this, i'll try to get to this by the end of the week

any chance you're adding some unit tests to the inline plugin you wrote until then?
thanks!

[jdalton]

@nirgit

any chance you're adding some unit tests to the inline plugin you wrote until then? thanks!

At the moment the plugin will warn if it misses an inline (you specify inlining something it doesn't have the source for) and will also throw an error for unsupported syntax (complete with snippet of code at the line and the a "^" under the identifier in question.

[nirgit]

since the tests pass, and it's possible to disable the inline plugin easily (thanks!) and since it's contained, i'm fine with approving this PR, although there are 2 things that are still bugging me:

1. i think that the inline-plugin.js should have a dedicated unit test, as this module is not trivial to understand, and will stay hard to maintain in the future (making changes with less confidence).
2. it is unclear to me how you tested the performance before and after. can you please be very detailed about it?
   fwiw, what i did was build master, then using another testing node module that imports it

// package.json for testing module
...
"dependencies": {
    "simple-xml-to-json": "file:../simple-xml-to-json"
  }

// the testing script
'use strict'

const fs = require('fs')
const {convertXML} = require("simple-xml-to-json/lib/simpleXmlToJson.min.js")
// const {convertXML} = require("simple-xml-to-json")

const fileName = 'mock2.xml'
const fileToParse = fs.readFileSync(__dirname + "/" + fileName, {encoding: 'utf8'})

const start = Date.now()
const loops = 5000
for (let i = 0; i < loops; i++) {
    convertXML(fileToParse)
}
const end = Date.now()
console.log('average time for converting XML = ' + ((end-start)/loops) + 'ms')

surprisingly, when i did the same on your branch, i didn't see any difference in performance, so i'm not sure what i'm missing :\

thanks @jdalton for your contribution and patience 🙏🏻

🌿

[nirgit]

shouldn't this be true?

[jdalton]

Making it true prevents replacement in case: statements in switchs. As long as we aren't assigning to the constants this is fine to be true.

[nirgit]

would it make more sense to rename match to source?

[jdalton]

In this case its simulating string replace where a pattern is matched and then capture groups like funcName and args follow.

[jdalton]

@nirgit

surprisingly, when i did the same on your branch, i didn't see any difference in performance, so i'm not sure what i'm missing :\

This branch is just adding the inline support but not yet taking advantage of it. That happens in the performance PR.