[extend #1191] Unnecessary alloc for XML, JSON, JSONP #1199

im-kulikov · 2018-10-01T11:41:25Z

Implement methods for

JSON
JSONP
XML

fixes for tests
cc @vishr @ribice

Benchmark:

var testUser = user{1, "Jon Snow"}

func BenchmarkAllocJSONP(b *testing.B) {
	e := New()
	req := httptest.NewRequest(POST, "/", strings.NewReader(userJSON))
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec).(*context)

	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		c.JSONP(http.StatusOK, "callback", testUser)
	}
}

func BenchmarkAllocJSON(b *testing.B) {
	e := New()
	req := httptest.NewRequest(POST, "/", strings.NewReader(userJSON))
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec).(*context)

	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		c.JSON(http.StatusOK, testUser)
	}
}

func BenchmarkAllocXML(b *testing.B) {
	e := New()
	req := httptest.NewRequest(POST, "/", strings.NewReader(userJSON))
	rec := httptest.NewRecorder()
	c := e.NewContext(req, rec).(*context)

	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		c.XML(http.StatusOK, testUser)
	}
}

Result

# git checkout PR-branch
→ go test -test.v -test.run ^$ -test.bench ^BenchmarkAlloc* -count 10 > new.txt
# git checkout master-branch
→ go test -test.v -test.run ^$ -test.bench ^BenchmarkAlloc* -count 10 > old.txt

→ benchstat old.txt new.txt
name          old time/op    new time/op    delta
AllocJSONP-8    1.98µs ±17%    1.50µs ±12%  -24.11%  (p=0.000 n=10+9)
AllocJSON-8     1.82µs ±20%    1.73µs ± 6%     ~     (p=0.481 n=10+10)
AllocXML-8      4.27µs ±27%    4.27µs ±10%     ~     (p=0.549 n=10+9)

name          old alloc/op   new alloc/op   delta
AllocJSONP-8      193B ± 0%      162B ± 0%  -16.06%  (p=0.000 n=10+10)
AllocJSON-8       174B ± 0%      143B ± 0%  -17.82%  (p=0.000 n=10+10)
AllocXML-8      4.88kB ± 0%    4.77kB ± 0%   -2.30%  (p=0.019 n=6+8)

name          old allocs/op  new allocs/op  delta
AllocJSONP-8      5.00 ± 0%      4.00 ± 0%  -20.00%  (p=0.000 n=10+10)
AllocJSON-8       3.00 ± 0%      2.00 ± 0%  -33.33%  (p=0.000 n=10+10)
AllocXML-8        11.0 ± 0%      10.0 ± 0%   -9.09%  (p=0.000 n=10+10)

codecov · 2018-10-01T11:44:56Z

Codecov Report

Merging #1199 into master will increase coverage by 0.25%.
The diff coverage is 82.92%.

@@            Coverage Diff             @@
##           master    #1199      +/-   ##
==========================================
+ Coverage   81.61%   81.86%   +0.25%     
==========================================
  Files          27       27              
  Lines        1931     1991      +60     
==========================================
+ Hits         1576     1630      +54     
- Misses        248      249       +1     
- Partials      107      112       +5

Impacted Files	Coverage Δ
context.go	`74.24% <82.92%> (-0.07%)`	⬇️
echo.go	`88.81% <0%> (+1.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba1891b...c939bc1. Read the comment docs.

im-kulikov · 2018-10-09T17:53:52Z

@alexaandru Hey, what do you think about it?

alexaandru · 2018-10-09T19:20:30Z

I think in this particular case it would be overkill to have separate functions. The idea with "bool params is a code smell" is that it introduces complexity and makes code harder to understand, but it's not something set in stone, context always matter. Here we have very small functions, the "complexity" added is negligible and definitely smaller than the complexity of adding separate functions.

I did look into stdlib briefly and I did have a hard time finding functions that use bools as params, but I did find a couple (in the few minutes that I spent on it): https://golang.org/pkg/compress/gzip/#Reader.Multistream and https://golang.org/pkg/net/http/#Server.SetKeepAlivesEnabled

That being said... we could "cheat" a little ;) We could eliminate the bool param entirely and do the indent decision based on indent param alone: if not empty, then do the indent step, otherwise not.

im-kulikov · 2018-10-09T19:24:03Z

Hm.. okay.. I’ll fix it

UPD: @alexaandru fixed

alexaandru · 2018-10-10T11:19:31Z

context.go

 }

 func (c *context) JSONPretty(code int, i interface{}, indent string) (err error) {
-	return c.jsonBlob(code, i, true, indent)
+	return c.jsonBlob(code, i, indent)


This looks risky. It works fine in JSON() where we control the indent ourselves, here however it messes things up :( If they want it pretty but with a blank indent (which I have no idea what it would do? just put each field on a separate line with no indent?) they will get it "not pretty". Should we return an error when the indent is blank for pretty? and also update the documentation to reflect that we expect a non blank indent to be provided for this function?

Yes, just checked, that's what it currently does, with a blank indent, it prints pretty with no indent:

{ "bar": 1, "foo": 42 }

We can either revert back to using bool OR we can convert indent in jsonBlob() to be a pointer to string. That way, we don't check if it is blank, we check if it is nil. If nil == ugly json, if present (even if blank) == pretty json with that ident. That would allow us to keep the existing behavior where we accept even a blank indent.

yeah, you'll right

fixed with indent pointer

Looks good to me 👍

Glad to hear.. waiting for @vishr review

add benchmarks

ribice · 2018-10-11T09:41:11Z

Looks good to me. Thanks for the benchmarks!

im-kulikov · 2018-10-11T09:43:45Z

✌️Thx

…ary_allocs # Conflicts: # context_test.go

im-kulikov · 2018-10-15T11:49:06Z

@alexaandru / @vishr hi there! there is any chance to be merged?

im-kulikov · 2018-11-23T13:02:16Z

@alexaandru / @vishr hi there! there is any chance to be merged?

im-kulikov · 2018-12-09T21:57:16Z

@vishr hi there, it is not required and possible to be closed?

there is no reaction for more than two months

im-kulikov · 2018-12-18T18:15:30Z

@vishr can I reopen PR?

vishr · 2018-12-18T18:17:21Z

It was a bot thing, please do it. It might take a while to get to it.

im-kulikov · 2018-12-18T18:19:19Z

I just reopen, as I can see, all tests passed and has no conflicts with master..

im-kulikov · 2019-01-14T12:56:39Z

@vishr ping?

vishr · 2019-01-14T18:01:18Z

context.go

@@ -206,6 +206,8 @@ const (
 	indexPage     = "index.html"
 )

+var defaultIndent = "  "


Can this be a const?

vishr · 2019-01-14T18:14:31Z

Don’t worry I will take care of it.

…

On Jan 14, 2019, at 10:13 AM, Evgeniy Kulikov ***@***.***> wrote: @im-kulikov commented on this pull request. In context.go <#1199 (comment)>: > @@ -206,6 +206,8 @@ const ( indexPage = "index.html" ) +var defaultIndent = " " My mistake — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#1199 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATKtDrVuZJZJq3lWWMfzkDgyQgoUB4sks5vDMjlgaJpZM4XB46c>.

im-kulikov · 2019-01-14T18:15:23Z

@vishr thx

ribice · 2019-01-14T18:25:17Z

Nice 👍

Signed-off-by: Vishal Rana <vr@labstack.com>

[extend #1191] Unnecessary alloc for XML, JSON, JSONP

d0b6d07

im-kulikov added 3 commits October 1, 2018 15:00

add legacy (JSON/JSONP/XML)Blob tests

3dc6c4f

fix namings

bb09993

fix jsonPBlob allocs

e332624

fix review comments (thx @alexaandru)

2cb6d3a

alexaandru reviewed Oct 10, 2018

View reviewed changes

fix review comments (thx @alexaandru)

521e4e0

add benchmarks

im-kulikov mentioned this pull request Oct 10, 2018

support jsoniter #1204

Closed

Merge branch 'master' of git://github.com/labstack/echo into unnecess…

c939bc1

…ary_allocs # Conflicts: # context_test.go

im-kulikov closed this Dec 13, 2018

im-kulikov deleted the unnecessary_allocs branch December 13, 2018 23:11

im-kulikov restored the unnecessary_allocs branch December 18, 2018 18:18

im-kulikov reopened this Dec 18, 2018

vishr approved these changes Jan 14, 2019

View reviewed changes

vishr merged commit 62145fa into labstack:master Jan 14, 2019

im-kulikov deleted the unnecessary_allocs branch January 14, 2019 18:15

vishr added a commit that referenced this pull request Jan 14, 2019

Changes related to #1199

7867fce

Signed-off-by: Vishal Rana <vr@labstack.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[extend #1191] Unnecessary alloc for XML, JSON, JSONP #1199

[extend #1191] Unnecessary alloc for XML, JSON, JSONP #1199

im-kulikov commented Oct 1, 2018 •

edited

Loading

codecov bot commented Oct 1, 2018 •

edited

Loading

im-kulikov commented Oct 9, 2018

alexaandru commented Oct 9, 2018

im-kulikov commented Oct 9, 2018 •

edited

Loading

alexaandru Oct 10, 2018

im-kulikov Oct 10, 2018

im-kulikov Oct 10, 2018

alexaandru Oct 10, 2018

im-kulikov Oct 10, 2018

ribice commented Oct 11, 2018

im-kulikov commented Oct 11, 2018

im-kulikov commented Oct 15, 2018 •

edited

Loading

im-kulikov commented Nov 23, 2018

im-kulikov commented Dec 9, 2018 •

edited

Loading

im-kulikov commented Dec 18, 2018

vishr commented Dec 18, 2018

im-kulikov commented Dec 18, 2018

im-kulikov commented Jan 14, 2019

vishr Jan 14, 2019

im-kulikov Jan 14, 2019

vishr commented Jan 14, 2019 via email

im-kulikov commented Jan 14, 2019

ribice commented Jan 14, 2019

[extend #1191] Unnecessary alloc for XML, JSON, JSONP #1199

[extend #1191] Unnecessary alloc for XML, JSON, JSONP #1199

Conversation

im-kulikov commented Oct 1, 2018 • edited Loading

Benchmark:

Result

codecov bot commented Oct 1, 2018 • edited Loading

Codecov Report

im-kulikov commented Oct 9, 2018

alexaandru commented Oct 9, 2018

im-kulikov commented Oct 9, 2018 • edited Loading

alexaandru Oct 10, 2018

Choose a reason for hiding this comment

im-kulikov Oct 10, 2018

Choose a reason for hiding this comment

im-kulikov Oct 10, 2018

Choose a reason for hiding this comment

alexaandru Oct 10, 2018

Choose a reason for hiding this comment

im-kulikov Oct 10, 2018

Choose a reason for hiding this comment

ribice commented Oct 11, 2018

im-kulikov commented Oct 11, 2018

im-kulikov commented Oct 15, 2018 • edited Loading

im-kulikov commented Nov 23, 2018

im-kulikov commented Dec 9, 2018 • edited Loading

im-kulikov commented Dec 18, 2018

vishr commented Dec 18, 2018

im-kulikov commented Dec 18, 2018

im-kulikov commented Jan 14, 2019

vishr Jan 14, 2019

Choose a reason for hiding this comment

im-kulikov Jan 14, 2019

Choose a reason for hiding this comment

vishr commented Jan 14, 2019 via email

im-kulikov commented Jan 14, 2019

ribice commented Jan 14, 2019

im-kulikov commented Oct 1, 2018 •

edited

Loading

codecov bot commented Oct 1, 2018 •

edited

Loading

im-kulikov commented Oct 9, 2018 •

edited

Loading

im-kulikov commented Oct 15, 2018 •

edited

Loading

im-kulikov commented Dec 9, 2018 •

edited

Loading