Remove duplicated Module.Route calls #7716

robert-zaremba · 2020-10-28T17:43:27Z

Description

When analyzing / debugging modules I found two weird things:

we do strings.TrimSpace("") -- this doesn't make any effect
we call Module.Route() twice (hence, 2 times construct gRPC) server.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

robert-zaremba · 2020-10-28T17:44:50Z

types/router.go

@@ -40,7 +40,7 @@ type Route struct {

 // NewRoute returns an instance of Route.
 func NewRoute(p string, h Handler) Route {
-	return Route{path: p, handler: h}
+	return Route{path: strings.TrimSpace(p), handler: h}


I was wondering if I should do the TrimSpace in line 58, but on r.path or just move it here.
I move it here for a hygiene.

This is OK 👍

robert-zaremba · 2020-10-28T17:45:36Z

types/router.go

@@ -55,7 +55,7 @@ func (r Route) Handler() Handler {

 // Empty returns true only if both handler and path are not empty.
 func (r Route) Empty() bool {
-	return r.handler == nil || r.path == strings.TrimSpace("")


strings.TrimSpace("") doesn't make any sense. It should be on path. I move trimming to line 43.

Makes sense

alessio · 2020-10-28T18:04:05Z

types/module/module.go

@@ -278,11 +278,11 @@ func (m *Manager) RegisterInvariants(ir sdk.InvariantRegistry) {
 // RegisterRoutes registers all module routes and module querier routes
 func (m *Manager) RegisterRoutes(router sdk.Router, queryRouter sdk.QueryRouter, legacyQuerierCdc *codec.LegacyAmino) {
 	for _, module := range m.Modules {
-		if !module.Route().Empty() {
-			router.AddRoute(module.Route())
+		if r := module.Route(); !r.Empty() {


I don't see much benefit here. Using methods allow the cache to kick in and you spare memory allocation. Not a big deal, it just looks a bit of a pointless change?

Yeah I don't think these changes are necessary. Makes it generally harder to read. My rule of thumb is typically, if a value is used more than once, put it in a variable. I'd revert these changes.

It is used more than once. See the line below. That's why I did that change. PRO TIP: see also the PR description.

PRO TIP: see also the PR description.

I saw it. Problem is that you didn't see my Using methods allow the cache to kick in and you spare memory allocation..
My advice is read it again. And again, should that help.

Please revert.

When imperative, not jitted methods are cached? Do you have any resource about it, especially related to Go? I would imagine that JIT could do it. But Go doesn't have a JIT.

You're absolutely right, my bad. I think this change set is fine OK.

Thanks @alexanderbez

@alessio - of course this is changeset is minor. Like I wrote, I did it because I was in this function while tracing an app and noticed it. If I wouldn't do the change with Trim, I would probably skip it. But it was few seconds to do that change.
Also, I don't believe that any function call cache validator will be smarter and faster than just using this reference - it's local and it will stay in the CPU cache anyway. Statically compiled, imperative languages don't do that - even on hardware, the the processor will need to know the whole context of the call. Especially here the module has a complex object and it can't mutate. To use a runtime function call cache you will need to assert that nothing changed. JIT or pure languages could do that.

I don't understand why do you like to bash the work I'm doing. I was proving that error checking is kind of critical. It's easier to do few not necessary error checks then be cautious to not forget a crucial check or wrongly assigned error. That's why we have tools for that. During the SDK call when I mentioned about errcheck nobody was against. Instead of spending hours of yours and mine time criticizing you could also write test cases.

Disclaimer #1: I've approved this PR already. Since the author seems to enjoy some pointless debate though, I feel compelled to entertain this circus for a tad longer.

Disclaimer #2: This is my last response in this thread. I have quite a number of things on my plate and I cannot afford any further meaningless distractions.

of course this is changeset is minor.

Is it? Sounds like a astdounding revelation. /me truly gobsmacked.

I don't believe that any function call cache validator will be smarter and faster than just using this reference

Whether you believe it or not does not change that you can't predict what happens at runtime beforehand, once it's compiled on either your machine, my machine or someone else's machine. Fact.

it's local and it will stay in the CPU cache anyway

That too proves another point of mine (just-a-cosmetic-change). Bingo (once more).

Statically compiled, imperative languages don't do that - even on hardware, the the processor will need to know the whole context of the call. Especially here the module has a complex object and it can't mutate.

Again, this proves nothing for you can't predict on all target platforms what happens at runtime unless you dig into the binary's assembly for each platform this tiny, insignificant piece of code will be compiled.

I approved this PR. I warmly advise against tilting at windmills. There is a whole ambitious Cosmos SDK testing tools & sandbox work plan drafted by this PR's author that will hardly be implemented by just staring at it.

'Been fun.

AT

That too proves another point of mine (just-a-cosmetic-change). Bingo (once more).

It is not. Don't quote fragments of my post without a context, please. r := funWithComplexDependencies(); doSomeithg(r) is totally different in not-jitted, imperative languages than funWithComplexDependencies(); doSomething(funWithComplexDependencies()).

types/module/module.go

alessio

This PR should add value. In my very humble opinion, it doesn't.

alexanderbez · 2020-10-29T18:48:41Z

types/module/module.go

@@ -278,11 +278,11 @@ func (m *Manager) RegisterInvariants(ir sdk.InvariantRegistry) {
 // RegisterRoutes registers all module routes and module querier routes
 func (m *Manager) RegisterRoutes(router sdk.Router, queryRouter sdk.QueryRouter, legacyQuerierCdc *codec.LegacyAmino) {
 	for _, module := range m.Modules {
-		if !module.Route().Empty() {
-			router.AddRoute(module.Route())
+		if r := module.Route(); !r.Empty() {


You're absolutely right, my bad. I think this change set is fine OK.

alexanderbez · 2020-10-29T18:49:05Z

types/router.go

@@ -40,7 +40,7 @@ type Route struct {

 // NewRoute returns an instance of Route.
 func NewRoute(p string, h Handler) Route {
-	return Route{path: p, handler: h}
+	return Route{path: strings.TrimSpace(p), handler: h}


This is OK 👍

alexanderbez · 2020-10-29T19:20:54Z

Looks like we need to fix TestManager_RegisterRoutes and we can get this bad boy merged!

codecov · 2020-10-29T21:53:54Z

Codecov Report

Merging #7716 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7716   +/-   ##
=======================================
  Coverage   54.21%   54.21%           
=======================================
  Files         611      611           
  Lines       38654    38654           
=======================================
  Hits        20956    20956           
  Misses      15564    15564           
  Partials     2134     2134

Remove duplicated Module.Route calls

82b7403

robert-zaremba requested review from aaronc, alessio and alexanderbez as code owners October 28, 2020 17:43

robert-zaremba requested review from amaury1093 and blushi October 28, 2020 17:43

robert-zaremba commented Oct 28, 2020

View reviewed changes

alessio reviewed Oct 28, 2020

View reviewed changes

types/module/module.go Show resolved Hide resolved

alessio suggested changes Oct 28, 2020

View reviewed changes

alexanderbez approved these changes Oct 29, 2020

View reviewed changes

alessio approved these changes Oct 29, 2020

View reviewed changes

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 29, 2020

make a proper test for registering empty route

4c8c473

robert-zaremba force-pushed the robert/route-opt branch from 275fdc4 to 4c8c473 Compare October 29, 2020 21:44

Merge branch 'master' into robert/route-opt

350f012

mergify bot merged commit e0e16f6 into master Oct 29, 2020

mergify bot deleted the robert/route-opt branch October 29, 2020 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove duplicated Module.Route calls #7716

Remove duplicated Module.Route calls #7716

robert-zaremba commented Oct 28, 2020

robert-zaremba Oct 28, 2020

alexanderbez Oct 29, 2020

robert-zaremba Oct 28, 2020

alessio Oct 28, 2020

alessio Oct 28, 2020 •

edited

Loading

alexanderbez Oct 28, 2020

robert-zaremba Oct 28, 2020 •

edited

Loading

alessio Oct 28, 2020

robert-zaremba Oct 28, 2020

alexanderbez Oct 29, 2020

robert-zaremba Oct 29, 2020

robert-zaremba Oct 29, 2020 •

edited

Loading

alessio Oct 29, 2020

robert-zaremba Oct 29, 2020 •

edited

Loading

alessio left a comment

alexanderbez Oct 29, 2020

alexanderbez Oct 29, 2020

alexanderbez commented Oct 29, 2020

codecov bot commented Oct 29, 2020

Remove duplicated Module.Route calls #7716

Remove duplicated Module.Route calls #7716

Conversation

robert-zaremba commented Oct 28, 2020

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robert-zaremba Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Oct 29, 2020

codecov bot commented Oct 29, 2020

Codecov Report

alessio Oct 28, 2020 •

edited

Loading

robert-zaremba Oct 28, 2020 •

edited

Loading

robert-zaremba Oct 29, 2020 •

edited

Loading

robert-zaremba Oct 29, 2020 •

edited

Loading