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

Optimizations for DiscreteTrajectory #2981

Merged
merged 12 commits into from
May 14, 2021

Conversation

rnlahaye
Copy link
Contributor

In particular,

  • DiscreteTrajectory::EvaluatePosition and friends now only compute the interpolation when necessary (as suggested by @pleroy in Downsampling is slow #2961).
  • Forkable::{begin, front, back} have been made more efficient.

Accordingly, DiscreteTrajectory benchmarks for t_max, t_min, and EvaluateDegreesOfFreedom have been added. Additionally, some unrelated DiscreteTrajectory benchmarks have been improved:

  • The find and lower_bound benchmarks now correctly adapt to trajectory size.
  • The iteration benchmarks now cache begin and end.

Here are the relevant benchmark diffs from fc81d60 to f52362a:

Benchmark                                                                   Time             CPU      Time Old      Time New       CPU Old       CPU New
--------------------------------------------------------------------------------------------------------------------------------------------------------
BM_DiscreteTrajectoryFront                                               -0.8568         -0.8565            23             3            23             3
BM_DiscreteTrajectoryBack                                                -0.6674         -0.6678            17             6            17             6
BM_DiscreteTrajectoryBegin                                               -0.2585         -0.2590            23            17            23            17
BM_DiscreteTrajectoryEnd                                                 +0.0118         +0.0112             5             5             5             5
BM_DiscreteTrajectoryTMin                                                -0.7728         -0.7728            27             6            27             6
BM_DiscreteTrajectoryTMax                                                -0.5384         -0.5391            19             9            19             9
BM_DiscreteTrajectoryEvaluateDegreesOfFreedomExact                       -0.7189         -0.7188            95            27            95            27
BM_DiscreteTrajectoryEvaluateDegreesOfFreedomInterpolated                -0.1196         -0.1186            93            82            93            82

@pleroy
Copy link
Member

pleroy commented May 10, 2021

[Automated message from GitHub Pull Request Builder] Answer "ok to test" to trigger testing of this PR.

@rnlahaye
Copy link
Contributor Author

ok to test

@pleroy
Copy link
Member

pleroy commented May 10, 2021

Jenkins says:

18:51:11     30>C:\Program Files (x86)\Jenkins\workspace\Principia\physics\forkable_body.hpp(205): error C2220: the following warning is treated as an error [C:\Program Files (x86)\Jenkins\workspace\Principia\physics\physics.vcxproj]
18:51:11     30>C:\Program Files (x86)\Jenkins\workspace\Principia\physics\forkable_body.hpp(205): warning C4172: returning address of local variable or temporary [C:\Program Files (x86)\Jenkins\workspace\Principia\physics\physics.vcxproj]
18:51:11     30>C:\Program Files (x86)\Jenkins\workspace\Principia\physics\forkable_body.hpp(213): warning C4172: returning address of local variable or temporary [C:\Program Files (x86)\Jenkins\workspace\Principia\physics\physics.vcxproj]

The conversion is now done by a static method instead of a constructor.
There is also some documentation now.
Also fixed inadvertently broken forkable_test.
@rnlahaye
Copy link
Contributor Author

ok to test

@rnlahaye
Copy link
Contributor Author

Any information on the latest error? I don’t see any errors with clang.

@pleroy
Copy link
Member

pleroy commented May 11, 2021

It's just lint now:

23:22:02 C:\Program Files (x86)\Jenkins\workspace\Principia\physics\discrete_trajectory_body.hpp(35):  Lines should very rarely be longer than 100 characters  [whitespace/line_length] [4]

Sorry, something went wrong.

@rnlahaye
Copy link
Contributor Author

ok to test

Sorry, something went wrong.

@@ -50,6 +50,9 @@ class DiscreteTrajectoryIterator
DegreesOfFreedom<Frame> const& degrees_of_freedom;
};

static reference MakeReference(
typename DiscreteTrajectoryTraits<Frame>::TimelineConstIterator it);
Copy link
Member

Choose a reason for hiding this comment

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

This factory is a bit tasteless. I would say that the initializer lists {it->first, it->second} that we had previously was perfectly readable (if slightly verbose) and did not clutter the API like this function does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{it->first, it->second} does not work for FakeTrajectoryIterator::reference (a.k.a. Instant const&) in forkable_test.cc. How do you recommend I proceed?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's annoying. Do you understand why Visual Studio was complaining about the constructor? That seemed like a better approach than the factory.

If the constructor that takes a TimelineConstIterator doesn't work, would a constructor that takes a Timeline::value_type (i.e., the pair itself) by reference work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it is because I forgot about FakeTrajectory and didn't handle it at all until 89ca90a.

I'm trying the constructor approach again but this time replacing FakeTrajectoryIterator::reference with a wrapper struct with the proper constructor. Hopefully Visual Studio is happy this time.

physics/discrete_trajectory.hpp Outdated Show resolved Hide resolved
physics/discrete_trajectory_body.hpp Outdated Show resolved Hide resolved
physics/discrete_trajectory_body.hpp Outdated Show resolved Hide resolved
benchmarks/discrete_trajectory.cpp Outdated Show resolved Hide resolved
benchmarks/discrete_trajectory.cpp Outdated Show resolved Hide resolved
physics/forkable_body.hpp Outdated Show resolved Hide resolved
@pleroy
Copy link
Member

pleroy commented May 13, 2021

Some more lint:

22:51:01 C:\Program Files (x86)\Jenkins\workspace\Principia\benchmarks\discrete_trajectory.cpp(41):  Missing space before {  [whitespace/braces] [5]
22:51:01 C:\Program Files (x86)\Jenkins\workspace\Principia\benchmarks\discrete_trajectory.cpp(55):  Missing space before {  [whitespace/braces] [5]

@pleroy pleroy added the LGTM label May 14, 2021
@pleroy
Copy link
Member

pleroy commented May 14, 2021

This looks fine now, other than one remaining lint error:

10:07:09 C:\Program Files (x86)\Jenkins\workspace\Principia\physics\forkable_body.hpp(223):  Lines should be <= 80 characters long  [whitespace/line_length] [2]

@pleroy
Copy link
Member

pleroy commented May 14, 2021

Thanks for your contribution. Merging.

@pleroy pleroy merged commit 74e4b07 into mockingbirdnest:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants