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

IRI resolution using user-provided buffers #6

Closed
lo48576 opened this issue Jan 3, 2022 · 4 comments
Closed

IRI resolution using user-provided buffers #6

lo48576 opened this issue Jan 3, 2022 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@lo48576
Copy link
Owner

lo48576 commented Jan 3, 2022

Currently IRI resolution implementation is provided only when alloc feature is enabled.
This is because the algorithm uses a (variable-size) stack to manipulate the IRI.

However, the maximum size of the necessary stack is easy to estimate, since the length of the resulting IRI won't exceed the sum of the lengths of the two inputs.
So, providing IRI resolution that utilize user-provided &[u8] buffer would be useful.

Additionally, a variation that utilize user-provided &mut String would also be useful, since it can reduce memory allocations when users attempts to resolve multiple IRIs at once.

@lo48576 lo48576 added the enhancement New feature or request label Jan 3, 2022
@lo48576 lo48576 self-assigned this Jan 3, 2022
@lo48576 lo48576 added the work in progress Work in progress label Jan 5, 2022
@lo48576
Copy link
Owner Author

lo48576 commented Jan 6, 2022

Done in 694555f (force pushed the fix).

I want more tests before releasing it.

@lo48576
Copy link
Owner Author

lo48576 commented Jan 7, 2022

    #[bench]
    fn bench_new_resolve(b: &mut test::Bencher) {
        let base = IriStr::new("https://sub.example.com/foo1/foo2/foo3/foo4/foo5")
            .expect("should be valid IRI");
        let rel = IriReferenceStr::new("bar1/bar2/bar3/../bar4/../../bar5/../../../../../bar6/../../../bar7/././././././bar8/bar9")
            .expect("should be valid IRI");
        b.iter(|| {
            let resolved = crate::resolve::resolve(rel, base);
            resolved
        });
    }

    #[bench]
    fn bench_old_resolve(b: &mut test::Bencher) {
        use crate::types::IriAbsoluteStr;

        let base = IriAbsoluteStr::new("https://sub.example.com/foo1/foo2/foo3/foo4/foo5")
            .expect("should be valid IRI");
        let rel = IriReferenceStr::new("bar1/bar2/bar3/../bar4/../../bar5/../../../../../bar6/../../../bar7/././././././bar8/bar9")
            .expect("should be valid IRI");
        b.iter(|| {
            let resolved = crate::resolve_old::resolve(rel, base, true);
            resolved
        });
    }
test resolve::tests::bench_new_resolve ... bench:       3,789 ns/iter (+/- 98)
test resolve::tests::bench_old_resolve ... bench:       3,288 ns/iter (+/- 58)

😩

@lo48576
Copy link
Owner Author

lo48576 commented Jan 7, 2022

Inspecting flamegraphs, I realized that the parsings are quite heavy.

New resolve() does one extra parsing compared to the old implementation.
ResolutionTask::write_to_buf() returns Result<&'b RiStr<S>, _> but drops the Ok value (as resolve() returns RiString<S>, not &RiStr<S>).
I changed that part to use unsafe and debug_assert, and tried benchmarking again.

// Previous impl:
    /// Resolves the IRI, and writes it to the buffer.
    fn write_to_buf<'b, B: Buffer<'b>>(&self, buf: B) -> Result<&'b RiStr<S>, B::ExtendError> {
        let s = self.common.write_to_buf(buf)?;
        // Convert the type.
        // This should never fail (unless the crate has bugs), but do the
        // validation here for extra safety.
        let s = <&RiStr<S>>::try_from(s).expect("[consistency] the resolved IRI must be valid");
        Ok(s)
    }
// Improved impl:
    /// Resolves the IRI, and writes it to the buffer.
    fn write_to_buf<'b, B: Buffer<'b>>(&self, buf: B) -> Result<&'b RiStr<S>, B::ExtendError> {
        let s = self.common.write_to_buf(buf)?;
        // Convert the type.
        // This should never fail (unless the crate has bugs), but do the
        // validation here for extra safety.
        debug_assert!(<&RiStr<S>>::try_from(s).is_ok(), "[consistency] the resolved IRI must be valid");
        let s = core::str::from_utf8(s).expect("[consistency] the resolved IRI must be valid");
        let s = unsafe { RiStr::<S>::new_maybe_unchecked(s) };
        Ok(s)
    }
test resolve::tests::bench_new_resolve ... bench:       3,327 ns/iter (+/- 30)
test resolve::tests::bench_old_resolve ... bench:       3,514 ns/iter (+/- 87)

This means that the resolution logic itself has not become slow, but extra parsing has simply slowed the function.
I'm going to use the new implementation in the next release.

@lo48576
Copy link
Owner Author

lo48576 commented Jan 7, 2022

Anyway, I'll make the parser faster.

lo48576 added a commit that referenced this issue Jan 7, 2022
@lo48576 lo48576 closed this as completed in aeb7dc0 Jan 7, 2022
@lo48576 lo48576 removed the work in progress Work in progress label Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant