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

Some refactoring, *Assign operators and box transform methods for Translations #423

Merged
merged 5 commits into from Apr 5, 2020

Conversation

@Mingun
Copy link
Contributor

Mingun commented Apr 3, 2020

As a continuation of our discourse about by value vs by reference: added translation methods for Boxes takes its by reference, but as godbolt shows, even without forced #[inline] directive compiler smart enough to inline such small function and the code the same in both cases. Therefore, I'm still in favour of taking everything by value in methods that almost only change unit. For now I'm, however, use reference as in surrounding code.

@nical
Copy link
Collaborator

nical commented Apr 5, 2020

As a continuation of our discourse about by value vs by reference: added translation methods for Boxes takes its by reference, but as godbolt shows, even without forced #[inline] directive compiler smart enough to inline such small function and the code the same in both cases.

I salute your approach of checking things in godbolt beforehand. It's the right thing to do, but there are a few traps to be aware of when drawing conclusions:

  • In general, inlining is a tricky optimization to simulate: the smaller/simpler your test programs/functions are, the easier it is for the compiler to inline things. It can depend on the function to inline but also where the function is potentially inlined.
  • Inlining is more likely to happen within a crate than when calling a public function or method defined in another crate. Rustc is particularly shy about inlining across crates and it's hard to observe this kind of effect in tools like godbolt.

Beyond that, there are simple and complex functions manipulating rects and boxes. I prefer to be consistent about how each type is passed (value or ref) basing on a simple rule (size of the struct) without penalizing functions that cant or shouldn't be inlined, than look at each function and try to see if a particular version of the compiler inlines them (with the risks of extrapolating from simplified test cases).

You'd be right to point out that there are some inconsistencies in euclid today (for example 2d points are passed by value in most places but there are a functions that pass them by ref), and these are good candidates for fixing in the next wave of breaking change.

Note that the size of the struct is the heuristic that static analyzers like clippy and clang tools in C++ land use to advise for or against passing copyable structures by value. I just had a look at clippy's source code and it uses 8 bytes as the threshold above which they don't nudge you towards passing structures by value.

Anyway, the patch looks good, thanks. @bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2020

📌 Commit d2e7437 has been approved by nical

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2020

Testing commit d2e7437 with merge 6b5ca1f...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 5, 2020

☀️ Test successful - checks-travis
Approved by: nical
Pushing 6b5ca1f to master...

@bors-servo bors-servo merged commit 6b5ca1f into servo:master Apr 5, 2020
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@Mingun Mingun deleted the Mingun:translation branch Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.