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

Move inlines from *.cpp to *_impl.h #397

Merged
merged 1 commit into from Jul 5, 2016

Conversation

@jakobj
Copy link
Contributor

jakobj commented Jun 13, 2016

This PR moves inlined functions from cpp files to the corresponding impl.h files
and fixes the relevant includes. I have also inlined a few functions from vp_manager as was indicated by some leftover todo item. I suggest @apeyser and @tammoippen as reviewers.

@jakobj
Copy link
Contributor Author

jakobj commented Jun 14, 2016

@heplesser could you maybe also have a look? i didn't propose you as a reviewer, since these are partially your changes from PR #221.

}

inline void
nest::ConnBuilder::check_synapse_params_( std::string syn_name,

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

This method should not be inlined, it is simply too large and the time required to execute it will certainly be signficant compared to call overhead.

@@ -453,126 +363,6 @@ nest::ConnBuilder::disconnect()
throw WrappedThreadException( *( exceptions_raised_.at( thr ) ) );
}

inline void
nest::ConnBuilder::single_connect_( index sgid,

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

This method is also far too long and complex for inlining.

}

inline void
ConnBuilder::check_synapse_params_( std::string syn_name,

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

See above.

}

inline void
ConnBuilder::single_connect_( index sgid,

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

See above.

}

inline void
ConnBuilder::skip_conn_parameter_( thread target_thread )

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

This one is probably borderline for inlining. I would generally think that anything that contains loops should probably not be inlined, except, maybe if the loop would be empty (begin()==end()) in most cases. That might actually be the case here.

{

inline void
NodeManager::prepare_node_( Node* n )

This comment has been minimized.

Copy link
@heplesser

heplesser Jun 14, 2016

Contributor

The typical workload of this function through the init_buffers() and calibrate() calls will be so large that there is not really much point in inlining.

@apeyser
Copy link
Contributor

apeyser commented Jun 14, 2016

@jakobj

There's no technical damage from unnecessary inlines, since most compilers will just create an anonymous static in each module where it's used, if it fails tests to be inline-able. But it does slow down compiles, increase the likelihood for include errors involving forward declarations and such, and generally make isolating pieces more likely.

Do we have a reason to believe that anything is gained by inlining this code? Logical arguments on usage patterns, performance measurements, suggestions about exposing the internals of templates for optimization or such?

And in fact... why were these functions already declared as inline inside a cpp file? I think I've missed something here regarding the history.

@heplesser
Copy link
Contributor

heplesser commented Jun 14, 2016

@jakobj Separating the inline changes is a good idea to avoid overloading #221 with even more stuff. There was one inline fix in #221, though, that is necessary for MyModule to link. I will check that in #221.

Some of the methods that you have inlined in this PR are too "heavy" to inlining in my opinion. Inlining means that the function body will be pasted into the code in every place the function is called. This avoids call overhead, but bloats code, also the executable code, which might actually slow down the program. Inlining is thus useful only for very compact functions, such as getters, setters and tests.

Another issue is code maintainability: As soon as a function is inlined, its code must be visible everywhere where the function is called. Thus, far more h-files need to be included and any change in such a h-file will require all files that include it to be recompiled. Another problem is that it is easy to forget that _impl.h files need to be included and this can lead to linking errors that can be hard to understand. Therefore, if at all possible, we should try to avoid _impl.h files.

@heplesser
Copy link
Contributor

heplesser commented Jun 14, 2016

@apeyser I think the original reason for the inline in the cpp-file was a somewhat naive assumption that "inlining makes code run faster" (unconditionally). I only discovered the problem when strange linkage problems occurred in my work on MyModule.

@jakobj
Copy link
Contributor Author

jakobj commented Jun 14, 2016

@apeyser as @heplesser already stated all functions except the ones from vp_manager were declared as inline in the cpp files so i moved them to the impl.h files to avoid compilation issues that @heplesser encountered in the mymodule example. i'll move the big ones back to the cpp files but i think inlining the vp_manager functions does indeed make sense since they are pretty small.

@heplesser
Copy link
Contributor

heplesser commented Jun 14, 2016

@jakobj Ok. Are the impl-files necessary or can they be avoided?

@jakobj
Copy link
Contributor Author

jakobj commented Jun 14, 2016

@heplesser for the single_disconnect_ we need the _impl.h since it has a call to kernel(). i have moved the other functions to the .h file.

@heplesser
Copy link
Contributor

heplesser commented Jun 15, 2016

@jakobj This looks good. There is a small chance, though, that the inlining will lead to trouble with MyModule. Therefore, I suggest that we wait until #221 is merged, then merge #221 into this PR, before merging this one.

@jakobj
Copy link
Contributor Author

jakobj commented Jun 15, 2016

@heplesser sounds good. i'll also squash the commits before this PR is merged into master. this looks a bit messy atm.

Moved inlined functions from .cpp files to the corresponding _impl.h files
and fix includes. Inlined a few functions of vp_manager.
@jakobj jakobj force-pushed the jakobj:remove_inlines_from_cpps branch from 167b265 to a6a87ed Jul 4, 2016
@jakobj
Copy link
Contributor Author

jakobj commented Jul 4, 2016

@heplesser now that #221 is merged, i have updated this PR. MyModule still compiles correctly, so i think everything is safe.

@heplesser
Copy link
Contributor

heplesser commented Jul 5, 2016

@jakobj 👍 from me. @apeyser or @sdiazpier, could one of you take a second look?

@apeyser
Copy link
Contributor

apeyser commented Jul 5, 2016

@jakobj @heplesser
Looks fine to me. I'd avoid making so many inlines without profiling in the future, beyond the obvious cases of wrapping members in getters, setters and such or universal mini-objects like timestamps or node id objects that wrap an integer or such. We could even be slowing things down by filling multiple pages with the same functions for compilers that don't do inter-module optimization, but more importantly from a design point of view it fills the headers with dependencies --- and it's painful moving things around when you discover that you suddenly need a circular dependency because of inlines.

@heplesser
Copy link
Contributor

heplesser commented Jul 5, 2016

@apeyser I take your comment as approval and will merge this PR shortly. I think you advice on inlining is good and we should heed it in the future. Generally, I believe, inline is always only a hint, and the compiler may create a function proper even when we specify inline.

@heplesser heplesser merged commit 1abb181 into nest:master Jul 5, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jakobj jakobj deleted the jakobj:remove_inlines_from_cpps branch Sep 13, 2018
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.