Cython Parser/Lexer

[jhgg]

Although it's a bit too early to start optimizing for speed. I think that implementing the parser and lexer in Cython would speed up parsing speed. Or maybe we want to plug in fb's C++ parser implementation?

[rawls238]

could be relevant but haven't dug into it enough: https://github.com/graphql/libgraphqlparser/pull/14

[mpaolini]

@jhgg this first version kind of works https://github.com/elastic-coders/py-graphqlparser

[jhgg]

Oh. Awesome. Hmm. Will need to look to see how viable it would be to make that a pluggable AST backend. o.O

[mpaolini]

Doh, I got all the packaging wrong in 0.0.1 ... It is fixed now (hopefully)

[jhgg]

Would it be possible to match the generated Cython as closely as possible to our AST and visitors? Trying to figure out path of least resistance.

Sent from my iPhone

On Sep 19, 2015, at 6:53 PM, Marco Paolini [email protected] wrote:

Doh, I got all the packaging wrong in 0.0.1 ... It is fixed now (hopefully)

— Reply to this email directly or view it on GitHub.

[mpaolini]

It's definitely doable I guess. What's your AST and visitor API? You can go ahead and open a pull request on my repo

[woodb]

It would be neat if we could do an conditional import, falling back on this project's parser. It might make more sense to just write a wrapper for py-graphqlparser in our project that hooks it all up... esp. out of interest with keeping current with libgraphqlparser...

[jhgg]

Writing a wrapper has an overhead. I think that we should try to eliminate all unnecessary overhead in the parse / lex / ast / visit / validate layer. I think a decision has to be made whether or not we want to maintain a pure Python version (for non cPython implementations) alongside a Cython implementation.

I can justify the need to port all the rules to Cython and hook them up with Cython bindings to facebook's libgraphqlparser as it will end up being a huge performance boost that we will definitely need to be competitive with the JS version - as cPython does not have a JIT.

[mpaolini]

@woodb maybe a wrapper is not even needed: py-graphqlparser is still experimental and I am open to changing the API at this stage.

@jhgg conditional import makes sense. See aiohttp and its multidict cython/pure python implementation

[mpaolini]

Just dumping some differences in the visitor/AST node api:

graphqlparser:

    def enter_variable_definition(self, node):
        name = node.get_variable().get_name().get_value()

graphqllib:

    def enter_VariableDefinition(self, node, key, parent, path, ancestors):
        name = node.variable.name.value

[dittos]

Does our visitor and graphqlparser's visitor provide same functionallity? Context informations (key, parent, ...) are required to do validation.

[mpaolini]

@dittos as of now, context information is not passed in by graphqlparser's visitor. I will look into it and see what can be done

[jhgg]

@mpaolini - ping me when you're available. let's see what we need to get this to fruition.

[jhgg]

So, running a cProfile ontop of the pytest suite is really telling. Doesn't look like there is much overhead in the parser or the lexer. So the question is -- is leveraging the C library for this really needed?