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

Lots of unnecessary copies of object_ref #4698

Open
2 tasks
legrosbuffle opened this issue Jul 9, 2024 · 2 comments
Open
2 tasks

Lots of unnecessary copies of object_ref #4698

legrosbuffle opened this issue Jul 9, 2024 · 2 comments
Labels
bug Something isn't working P-high We will work on this issue

Comments

@legrosbuffle
Copy link
Contributor

Prerequisites

Please put an X between the brackets as you perform the following steps:

Description

Move constructors for objects deriving from object_ref in lean are making a copy, probably due to the assumption that an rvalue reference is propagated as such across function calls.

Example:

expr(expr && other):object_ref(other)

should be:

expr(expr && other):object_ref(std::move(other))

As a result, there are a lot of unnecessary incref/decref calls on object_ref. We've measured that expr copying and assignment by itself uses 10% of total runtime on our workloads.

Context

No specific context.

Steps to Reproduce

Expected behavior:

expr::expr(expr&& other) calls object_ref::object_ref(object_ref&&), which steals refcount from other, no incref happens.

Actual behavior:

expr::expr(expr&& other) calls object_ref::object_ref(const object_ref&), which increfs other's refcount. Then the callee decrefs other's refcount.

Versions

v4.7.0

Additional Information

n/a

Impact

Performance only.

@legrosbuffle legrosbuffle added the bug Something isn't working label Jul 9, 2024
legrosbuffle added a commit to legrosbuffle/lean4 that referenced this issue Jul 9, 2024
…rators

Right now those constructors result in a copy instead of the desired
move. We've measured that expr copying and assignment by itself uses
around 10% of total runtime on our workloads.

See leanprover#4698 for details.
legrosbuffle added a commit to legrosbuffle/lean4 that referenced this issue Jul 9, 2024
Those represent ~13% of the time spent in `save_result`,
even though `r` is a temporary in all cases but one.

See leanprover#4698 for details.
@leanprover-bot leanprover-bot added the P-high We will work on this issue label Aug 2, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 2, 2024
#4700)

…rators

Right now those constructors result in a copy instead of the desired
move. We've measured that expr copying and assignment by itself uses
around 10% of total runtime on our workloads.

See #4698 for details.
github-merge-queue bot pushed a commit that referenced this issue Aug 2, 2024
Those represent ~13% of the time spent in `save_result`,
even though `r` is a temporary in all cases but one.

See #4698 for details.

---------

Co-authored-by: Leonardo de Moura <[email protected]>
@eric-wieser
Copy link
Contributor

@legrosbuffle, thanks for this; can this issue be closed, or do you intend to track more improvements with this issue?

@legrosbuffle
Copy link
Contributor Author

I have a few more :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P-high We will work on this issue
Projects
None yet
Development

No branches or pull requests

3 participants