Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Xml ast improvements #376

Closed
wants to merge 8 commits into from
Closed

Xml ast improvements #376

wants to merge 8 commits into from

Conversation

TwitchBronBron
Copy link
Member

@TwitchBronBron TwitchBronBron commented Mar 30, 2021

This is part of the large set of breaking changes PR series (including #329).
Depends on #375

Notable changes:

  • Add location tracking (range property) to all SceneGraph/XML nodes.
  • Remove redundant SG transpile() functions in favor of the function from the base class.
    • This has the added benefit of including all valid XML from the source in the transpiled output. Prior to this PR, only known xml attributes and elements were included, but then had the potential to break any beta features that Roku might be testing.
  • Refactored SGComponent scripts, children, and interface properties into getter functions which are driven by the underlying XML AST. This means as you modify the AST, those getter functions will automatically reflect the changes.
  • Refactored SGInterface properties fields and functions to getter functions which are driven by the underlying XML AST (same benefits as previous point).
  • Moved SGParser module functions onto the SGParser class itself to match the OO patterns in the rest of the project.

The additional location tracking does result in a performance slowdown for XML parsing. However, considering XML files are generally pretty small (compared to brs files) and also considering that xml parsing is still over 200% faster than brs parsing, the XML location tracking is well worth the performance hit. However, I will be running a node profiler to see if I can squeeze out any additional performance.

image

@TwitchBronBron
Copy link
Member Author

I made a few small performance improvements to the SGParser which increased the parse-xml from 84 ops/sec to 99 ops/sec, at the expense of a slightly slower transpile speed (from 124 ops/sec to 100 ops/sec). So then I modified the TranspileState.transpileToken to be monomorphic (i.e. return a SourceNode every time instead of sometimes a string) and that gave back the lost transpile performance due to v8 optimizations.

I refactored the logic to find the XML node's bounding range to be computed on-demand instead of during every parse.

image

@elsassph
Copy link
Contributor

We need to ensure also that we support "unknown" tags and attributes everywhere.

default:
const nodeContent = mapNodes(content);
return new SGNode(name, attributes, nodeContent, range);
mapElement({ children }: ElementCstNode): SGTag {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like seeing all these pure functions moved into the classe but well...

@TwitchBronBron
Copy link
Member Author

Closed in favor of #400

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants