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

Speedup with Cython and Turtle parsing. #1263

Closed
wants to merge 10 commits into from
Closed

Speedup with Cython and Turtle parsing. #1263

wants to merge 10 commits into from

Conversation

rchateauneu
Copy link
Contributor

Fixes #

Proposed Changes

@coveralls
Copy link

coveralls commented Feb 26, 2021

Coverage Status

Coverage increased (+0.09%) to 75.507% when pulling 205bff2 on rchateauneu:master into 9f0f296 on RDFLib:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 75.507% when pulling 860f985 on rchateauneu:master into 9f0f296 on RDFLib:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 75.507% when pulling 860f985 on rchateauneu:master into 9f0f296 on RDFLib:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 75.507% when pulling 860f985 on rchateauneu:master into 9f0f296 on RDFLib:master.

i = i + 1
else:
break
#ln = c
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove unused code

@@ -1570,8 +1585,8 @@ def strconst(self, argstr, i, delim):
if ch == delim1:
j = i
continue
elif ch in ('"', "'") and ch != delim1:
ustr = ustr + ch
elif ch in "\"'" and ch != delim1:
Copy link
Contributor

Choose a reason for hiding this comment

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

To me the original looks more readable and - at least in cpython - it is actually faster. Didn't test cython though.

Copy link
Contributor Author

@rchateauneu rchateauneu Feb 27, 2021

Choose a reason for hiding this comment

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

Indeed the effect of replacing "x = x + y" by "x += y" is very small. However, other changes such as qname() and skipSpace() have a significant effect on my platform.

@FlorianLudwig
Copy link
Contributor

Overall this looks like a good MR to me. A lot of cleanup and cython 👍

I am curious why travis fails on powerpc - I cannot see any errors in the logs. Is it just me being blind?

@FlorianLudwig
Copy link
Contributor

FlorianLudwig commented Feb 27, 2021

It might be a good idea to split this up into two merge requests, one for the cleanup of the python code and one for setting up cython.

@rchateauneu
Copy link
Contributor Author

rchateauneu commented Feb 27, 2021

It might be a good idea to split this up into two merge requests, one for the cleanup of the python code and one for setting up cython.

Yes, why not. I am discovering rdflib code, so sorry for these errors.

Please do not hesitate to separately take any change you think is worth.

@rchateauneu
Copy link
Contributor Author

Overall this looks like a good MR to me. A lot of cleanup and cython +1

I am curious why travis fails on powerpc - I cannot see any errors in the logs. Is it just me being blind?

And, I restored setup.py to what it was, and I get the same error message on ppc only:
/plugins/serializers/nt.py:28: UserWarning: NTSerializer does not use custom encoding.

warnings.warn("NTSerializer does not use custom encoding.")

/home/travis/build/rchateauneu/rdflib/rdflib/plugins/serializers/nquads.py:24: UserWarning: NQuadsSerializer does not use custom encoding.

warnings.warn("NQuadsSerializer does not use custom encoding.")

.....

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

@FlorianLudwig
Copy link
Contributor

@ddeka2910 can you help here?

@rchateauneu
Copy link
Contributor Author

rchateauneu commented Feb 28, 2021

@ddeka2910
Thanks Ludwig and Debabrata.

I have recreated a new clone from scratch, but the problem still happens.

Please see the build here:
https://travis-ci.com/github/rchateauneu/rdflib/builds/218461927

@white-gecko white-gecko marked this pull request as draft March 1, 2021 13:45
@white-gecko
Copy link
Member

I have converted this one to a draft as the turtle parsing part is covered in #1266 and this one should be changes to only cover the cython improvements.

@rchateauneu
Copy link
Contributor Author

If you want, I will resend a specific PR for Cython, with changes only for setup.py .

@white-gecko
Copy link
Member

That would be great

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.

4 participants