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

importer: Migrate to ElementTree API #15

Merged
merged 1 commit into from
Feb 11, 2023
Merged

Conversation

bearzly
Copy link
Contributor

@bearzly bearzly commented Feb 11, 2023

The ElementTree API is significantly faster than the minidom API, resulting in ~60% faster imports.

Major differences:

  • Handling of XML namespaces (in minidom you would look for "spirit:name" but in ET you look for "{http://my/ns}name".
  • ET does not provide a direct way to get the 'localName', so a helper was added to keep the code the same
  • ET treats elements with no children as false-y, so some checks need to change to explicitly check for None

Besides those and some minor differences in the query API, this is essentialy a drop-in replacment.

The ElementTree API is significantly faster than the minidom API,
resulting in ~60% faster imports.

Major differences:
* Handling of XML namespaces (in minidom you would look for
  "spirit:name" but in ET you look for "{http://my/ns}name".
* ET does not provide a direct way to get the 'localName', so a helper
  was added to keep the code the same
* ET treats elements with no children as false-y, so some checks need to
  change to explicitly check for None

Besides those and some minor differences in the query API, this is
essentialy a drop-in replacment.
@bearzly
Copy link
Contributor Author

bearzly commented Feb 11, 2023

Benchmarked with python -m timeit -s 'from systemrdl import RDLCompiler; from peakrdl_ipxact import IPXACTImporter' 'rdlc = RDLCompiler(); ipxact = IPXACTImporter(rdlc); ipxact.import_file("some_ipxact.xml")'

Results from the 4 test XML files, as well as a vendor XML file that is 297 MB and has 73k registers

IP-XACT file Before After
generic_example_2009 20 loops, best of 5: 10.2 msec per loop 50 loops, best of 5: 4.44 msec per loop
generic_example_2014 20 loops, best of 5: 10.7 msec per loop 50 loops, best of 5: 4.6 msec per loop
nested_2009 5 loops, best of 5: 38.8 msec per loop 20 loops, best of 5: 16.8 msec per loop
nested_2014 5 loops, best of 5: 41.5 msec per loop 20 loops, best of 5: 17.5 msec per loop
giant 1 loop, best of 5: 114 sec per loop 1 loop, best of 5: 47.5 sec per loop

@bearzly bearzly marked this pull request as ready for review February 11, 2023 00:49
Copy link
Member

@amykyta3 amykyta3 left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for doing this

@amykyta3 amykyta3 merged commit aa05df1 into SystemRDL:main Feb 11, 2023
@bearzly
Copy link
Contributor Author

bearzly commented Feb 12, 2023

Thanks for merging this so quickly! We work with a lot of massive IP-XACT files so I might come back with some more performance improvements in the future.

@bearzly bearzly deleted the et branch February 12, 2023 01:45
@amykyta3
Copy link
Member

amykyta3 commented Feb 12, 2023

No problem!
I'll hopefully deploy it within this coming week, but need to check compatibility against a few things first. A few tools I work with use this and extend/augment the behavior of the importer via vendor extensions.

@amykyta3
Copy link
Member

Published in latest release.
Thanks again!

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