Feature: grammar helper about sigma HOT 11 CLOSED

For the record, opened an issue here to find out if this benchmark is still valid.

from sigma.

I wanted to know if this approach was faster or slower - it is slower, and I did attempt some optimizations, but I'm not an expert, and the benchmark is really unreliable on my system for some reason... I'm seeing anywhere from 1% to 10% overhead compared with defer, so I really have no idea if any of the optimizations I tried were making things faster or slower - so I left it the way it is for now.

On this branch, you can also see what the JSON parser would look like in this format.

mindplay-dk/sigma@add-core-folder...mindplay-dk:sigma:grammar-helper

I quite like the encapsulation, the type-safety, and removing the potential for uninitialized parsers - but I'm not sure what the actual overhead is, and so I can't really say if I think it's worth while, or whether it could be optimized.

There might be some inherent overhead due to the use of this in the JSON parser implementation itself, not sure... 🤔

from sigma.

Here's a simplified version:

https://github.com/mindplay-dk/sigma/blob/605780ab4ce6eb7d8bc335c62b7c2b7acdcd88c1/src/core/grammar.ts#L13-L29

export function grammar<T extends GrammarType>(init: GrammarInit<T>): Grammar<T> {
  const grammar = {} as { [key: string]: Parser<unknown> }

  for (const key in init) {
    grammar[key] = {
      parse() {
        throw new Error()
      }
    }
  }

  for (const key in init) {
    grammar[key].parse = init[key].apply(grammar).parse
  }

  return grammar as Grammar<T>
}

Just putting in "dummy" parsers for everything in the first loop, so that we get objects with a parse property that other objects can refer to, just to establish the circular references - then replace the parse property in a separate loop, "patching" the parsers.

Logically, this should be faster, because we're no longer doing anything dynamic - but it's quite possible that replacing a function like this breaks some sort of optimization in V8, I really can't say without a benchmark.

What's really suspicious, is if I swap the order of the benchmarks, so that grammar goes before defer rather than after, grammar consistently wins out by the same 10-15% margin - so there is something wrong with the benchmark, I think. 🤨

from sigma.

So yeah, I have to conclude the benchmark package (used by benny) probably isn't reliable anymore.

caderek/benny#57 (comment)

Not sure how to address this.

Either way, having run the benchmarks in isolation, I'm now certain the grammar version of the JSON parser is in fact slightly faster than the defer version.

So I think it makes sense to add this feature?

If you agree, I'll try to get it wrapped up with tests and docs soon 🙂

from sigma.

Yeah, benchmarking in Node/JS is super flaky, and benchmark.js seems to add headache in some cases. Not sure what we can do here (roll our own benchmarking library, perhaphs? 😁)

As for the feature itself, it does looks interesting and the implementation is kinda clever! Let's give it a go.

from sigma.

Yeah, benchmarking in Node/JS is super flaky, and benchmark.js seems to add headache in some cases. Not sure what we can do here (roll our own benchmarking library, perhaphs? 😁)

Yeah, I was tempted already and start mucking about with some maths here:

https://github.com/mindplay-dk/sigma/blob/crazy-math/benchmarks/src/json/index.ts

There isn't really anything maths can do to fix this problem though - after all the testing I did over the last couple of days, I'm fairly confident there is some sort of V8/JS engine optimization that applies only to the first module that loads.

Not sure where I'm going with this - if I decide to do something about this, it would need to have process isolation somehow, which probably means it'll have limitations on what you can test (e.g. Workers don't expose the DOM) or it'll work on Node only. (forking or launching a dedicated process for each test.)

I might do another round at some point and scout for an existing benchmark library that works...

As for the feature itself, it does looks interesting and the implementation is kinda clever! Let's give it a go.

I will try to get this wrapped up soon. 🙂👍

from sigma.

... aha! 🤔

from sigma.

Nope, couldn't get that to work.

Llorx/iso-bench#1

We'll see if the author responds - it's a very new project, but it does sound really promising. 🙂

from sigma.

Updated https://github.com/Llorx/iso-bench with an API easier to implement 🙂 Just commented on the issue there with your code @mindplay-dk working with the old API and also it working with the new API.

from sigma.

Thanks @Llorx 🙂👍

@norskeld lets consider the benchmark issue as a separate issue from the grammar helper, yeah?

from sigma.

Sure, okay.

from sigma.