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

Addition of unsafe calls #10

Closed
dbeacham opened this issue May 22, 2017 · 8 comments
Closed

Addition of unsafe calls #10

dbeacham opened this issue May 22, 2017 · 8 comments

Comments

@dbeacham
Copy link
Contributor

When processing a large XML document with many attributeBy and children calls I see a massive speed improvement if I import the FFI calls as unsafe.

I don't know if these changes would want to be made directly to the existing FFI imports or if unsafe variants should be introduced but it would be a useful addition to the library.

@ndmitchell
Copy link
Owner

What invariants do unsafe FFI calls have to obey? Reading around, I think all the functions could be marked unsafe as I never call back into Haskell - is that your understanding? If so, I'd welcome a patch to add unsafe to the standard ones (or will do it myself given a bit more time).

@ndmitchell
Copy link
Owner

Also, when you say "massive" are we talking 10% or 10x? I think it's a good idea even if it's only 1%, but curious for my own knowledge.

@dbeacham
Copy link
Contributor Author

It'll be very dependent on the XML you're running over but for my case I'm seeing

  • 10x improvement adding unsafe to attributeBy
  • 1.5x improvement adding unsafe to children

I think the basic guidance for adding unsafe is to make sure that they are small, pure, non-blocking functions.

This post and this github issue give a quick overview of where there can be complications but I don't think there are any such issues here.

I'll send over a pull request.

@dbeacham dbeacham reopened this May 22, 2017
@ndmitchell
Copy link
Owner

My guess is we should add unsafe on:

hexml_document_error
hexml_document_node
hexml_node_children
hexml_node_attributes
hexml_node_child
hexml_node_attribute

But not:

hexml_document_parse
hexml_document_free
hexml_node_render

What do you think?

dbeacham pushed a commit to dbeacham/hexml that referenced this issue May 22, 2017
dbeacham pushed a commit to dbeacham/hexml that referenced this issue May 22, 2017
@dbeacham
Copy link
Contributor Author

I agree. Although I suspect

hexml_document_error
hexml_document_node

won't see much benefit as they're only called once per document and so the overheads of safe calls don't add up.

@ndmitchell
Copy link
Owner

Yep, sounds right.

ndmitchell added a commit that referenced this issue May 22, 2017
Fix #10: Import common FFI calls as unsafe
ndmitchell added a commit that referenced this issue May 22, 2017
@ndmitchell
Copy link
Owner

Thanks! I'll make a release tonight.

@ndmitchell
Copy link
Owner

Released as 0.3.2. Thanks once 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

No branches or pull requests

2 participants