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.

Sign up for GitHub

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

Jump to bottom

Make bundle safe for concurrent writes #54

Closed
emosbaugh opened this issue Oct 27, 2016 · 8 comments
Closed

Make bundle safe for concurrent writes #54

emosbaugh opened this issue Oct 27, 2016 · 8 comments
Labels
enhancement

Comments

@emosbaugh
Copy link
Contributor

emosbaugh commented Oct 27, 2016 •
edited
Loading

Problem

Adding to the bundle in a concurrent program causes panics

Resolution

Make bundle safe for concurrent writes

Stack trace

fatal error: concurrent map read and map write

goroutine 49987 [running]:
runtime.throw(0x36a3440, 0x21)
    /usr/local/go/src/runtime/panic.go:547 +0x90 fp=0xc821f58bc0 sp=0xc821f58ba8
runtime.mapaccess1_faststr(0x28527e0, 0xc8203d3ef0, 0x3458300, 0xd, 0xc8203c5338)
    /usr/local/go/src/runtime/hashmap_fast.go:202 +0x5b fp=0xc821f58c20 sp=0xc821f58bc0
github.com/-/-/vendor/github.com/nicksnyder/go-i18n/i18n/bundle.(*Bundle).translate(0xc82040cc90, 0xc8203f4d60, 0x3458300, 0xd, 0x0, 0x0, 0x0, 0x0, 0x0)
    /go/src/github.com/-/-/vendor/github.com/nicksnyder/go-i18n/i18n/bundle/bundle.go:237 +0x15c fp=0xc821f58ce8 sp=0xc821f58c20
@emosbaugh emosbaugh changed the title Make bundle safe for concurrent access Make bundle safe for concurrent writes Oct 27, 2016
@nicksnyder
Copy link
Owner

Thanks for reporting this. You are correct go-i18n is not threadsafe. This is alluded to in a few locations in the docs, but perhaps it could be more explicit.

In the i18n.go package documentation:

Your Go program should load translations during its initialization.

And in the doc for LoadTranslationFile:

Generally you should load translation files once during your program's initialization.

Can you describe what your use-case is for concurrently reading and adding translations?

@emosbaugh
Copy link
Contributor Author

my applicated loads default translations at init as well as customized translations from the database. its not my intention to concurrently load translations. it just seems to be a symptom of the design of my app. for now i have added a mutex around calls to AddTranslation. i will submit a pr if i have some time.

emosbaugh added a commit to emosbaugh/go-i18n that referenced this issue Dec 2, 2016
@emosbaugh
Fixes nicksnyder#54 - make bundle safe for concurrent map reads and w…
b221fe1 
…rites
emosbaugh added a commit to emosbaugh/go-i18n that referenced this issue Dec 2, 2016
@emosbaugh
Fixes nicksnyder#54 - make bundle safe for concurrent map reads and w…
10af593 
…rites
emosbaugh added a commit to emosbaugh/go-i18n that referenced this issue Dec 2, 2016
@emosbaugh
Fixes nicksnyder#54 - make bundle safe for concurrent map reads and w…
d90aa2f 
…rites
emosbaugh added a commit to emosbaugh/go-i18n that referenced this issue Dec 2, 2016
@emosbaugh
Fixes nicksnyder#54 - make bundle safe for concurrent map reads and w…
d2b7f32 
…rites
@emosbaugh
Copy link
Contributor Author

@nicksnyder please take a look at my pr #59. benchmarks seem about the same.

@ArthurHlt ArthurHlt mentioned this issue Dec 2, 2016
Closed
@ArthurHlt
Copy link

I have the same issue, but I'm indirectly concerned, this project come from one of my repository .

I have made an issue linked to this one, I hope it can help to have some other feedbacks and usecases to make this issue fixed.

Let me know @nicksnyder if you need more context

@nicksnyder
Copy link
Owner

@emosbaugh Can you post before and after benchmark results?

@ArthurHlt
Copy link

@nicksnyder I did it for him to help a little.
I ran go test ./... -bench=. -cpu=1,2 here my results:

Before the change for concurrent writes

PASS
ok  	github.com/nicksnyder/go-i18n/goi18n	0.119s
PASS
ok  	github.com/nicksnyder/go-i18n/i18n	0.015s
BenchmarkTranslateNonPluralWithMap               	  500000	      2125 ns/op
BenchmarkTranslateNonPluralWithMap-2             	 1000000	      1869 ns/op
BenchmarkTranslateNonPluralWithStruct            	  500000	      2548 ns/op
BenchmarkTranslateNonPluralWithStruct-2          	  500000	      2588 ns/op
BenchmarkTranslateNonPluralWithStructPointer     	  500000	      3049 ns/op
BenchmarkTranslateNonPluralWithStructPointer-2   	  500000	      3075 ns/op
BenchmarkTranslatePluralWithMap                  	 1000000	      2367 ns/op
BenchmarkTranslatePluralWithMap-2                	 1000000	      2478 ns/op
BenchmarkTranslatePluralWithMapAndCountField     	  500000	      2545 ns/op
BenchmarkTranslatePluralWithMapAndCountField-2   	 1000000	      2276 ns/op
BenchmarkTranslatePluralWithStruct               	  500000	      3402 ns/op
BenchmarkTranslatePluralWithStruct-2             	  500000	      3358 ns/op
BenchmarkTranslatePluralWithStructPointer        	  500000	      3445 ns/op
BenchmarkTranslatePluralWithStructPointer-2      	  500000	      3443 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/bundle	24.220s
BenchmarkNewOperand     	 3000000	       445 ns/op
BenchmarkNewOperand-2   	 3000000	       450 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/language	3.630s
?   	github.com/nicksnyder/go-i18n/i18n/language/codegen	[no test files]
BenchmarkExecuteNilTemplate            	300000000	         4.65 ns/op
BenchmarkExecuteNilTemplate-2          	300000000	         4.59 ns/op
BenchmarkExecuteHelloWorldTemplate     	300000000	         4.67 ns/op
BenchmarkExecuteHelloWorldTemplate-2   	300000000	         4.63 ns/op
BenchmarkExecuteHelloNameTemplate      	 1000000	      1528 ns/op
BenchmarkExecuteHelloNameTemplate-2    	 1000000	      1590 ns/op
BenchmarkSprintf                       	10000000	       224 ns/op
BenchmarkSprintf-2                     	10000000	       221 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/translation	15.536s

After the change

PASS
ok  	github.com/nicksnyder/go-i18n/goi18n	0.025s
PASS
ok  	github.com/nicksnyder/go-i18n/i18n	0.011s
BenchmarkTranslateNonPluralWithMap               	  500000	      2092 ns/op
BenchmarkTranslateNonPluralWithMap-2             	 1000000	      2196 ns/op
BenchmarkTranslateNonPluralWithStruct            	  500000	      2788 ns/op
BenchmarkTranslateNonPluralWithStruct-2          	  500000	      2803 ns/op
BenchmarkTranslateNonPluralWithStructPointer     	  500000	      3239 ns/op
BenchmarkTranslateNonPluralWithStructPointer-2   	  500000	      3253 ns/op
BenchmarkTranslatePluralWithMap                  	  500000	      3153 ns/op
BenchmarkTranslatePluralWithMap-2                	  500000	      2934 ns/op
BenchmarkTranslatePluralWithMapAndCountField     	 1000000	      2694 ns/op
BenchmarkTranslatePluralWithMapAndCountField-2   	  500000	      2929 ns/op
BenchmarkTranslatePluralWithStruct               	  300000	      4251 ns/op
BenchmarkTranslatePluralWithStruct-2             	  500000	      3639 ns/op
BenchmarkTranslatePluralWithStructPointer        	  500000	      3734 ns/op
BenchmarkTranslatePluralWithStructPointer-2      	  300000	      3854 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/bundle	23.116s
BenchmarkNewOperand     	 3000000	       453 ns/op
BenchmarkNewOperand-2   	 3000000	       453 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/language	3.664s
?   	github.com/nicksnyder/go-i18n/i18n/language/codegen	[no test files]
BenchmarkExecuteNilTemplate            	300000000	         4.69 ns/op
BenchmarkExecuteNilTemplate-2          	300000000	         4.73 ns/op
BenchmarkExecuteHelloWorldTemplate     	300000000	         4.65 ns/op
BenchmarkExecuteHelloWorldTemplate-2   	300000000	         4.70 ns/op
BenchmarkExecuteHelloNameTemplate      	 1000000	      1546 ns/op
BenchmarkExecuteHelloNameTemplate-2    	 1000000	      1586 ns/op
BenchmarkSprintf                       	10000000	       224 ns/op
BenchmarkSprintf-2                     	 5000000	       264 ns/op
PASS
ok  	github.com/nicksnyder/go-i18n/i18n/translation	14.746s

As I expected it seems that his changes will have no impact (the result even show that it's faster with the changes but I just ran benchmark once, I consider that we will have the same time in both case).

The only impact we can have it's when concurrent writing occurred.

Hope it helps.

@emosbaugh
Copy link
Contributor Author

Seems on par with the current benchmarks. For me they run a bit slower although could be due to outside factors as i am running on my mac im using for dev.

$ git checkout master
Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
$ go test ./... -bench=. -cpu=1,2
PASS
ok  	github.com/nicksnyder/go-i18n/goi18n	0.019s
PASS
ok  	github.com/nicksnyder/go-i18n/i18n	0.007s
PASS
BenchmarkTranslateNonPluralWithMap            	 1000000	      1254 ns/op
BenchmarkTranslateNonPluralWithMap-2          	 1000000	      1174 ns/op
BenchmarkTranslateNonPluralWithStruct         	 1000000	      1660 ns/op
BenchmarkTranslateNonPluralWithStruct-2       	 1000000	      1495 ns/op
BenchmarkTranslateNonPluralWithStructPointer  	 1000000	      1731 ns/op
BenchmarkTranslateNonPluralWithStructPointer-2	 1000000	      1589 ns/op
BenchmarkTranslatePluralWithMap               	 1000000	      1670 ns/op
BenchmarkTranslatePluralWithMap-2             	 1000000	      1565 ns/op
BenchmarkTranslatePluralWithMapAndCountField  	 1000000	      1542 ns/op
BenchmarkTranslatePluralWithMapAndCountField-2	 1000000	      1425 ns/op
BenchmarkTranslatePluralWithStruct            	 1000000	      2069 ns/op
BenchmarkTranslatePluralWithStruct-2          	 1000000	      1959 ns/op
BenchmarkTranslatePluralWithStructPointer     	 1000000	      2105 ns/op
BenchmarkTranslatePluralWithStructPointer-2   	 1000000	      2000 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/bundle	23.513s
PASS
BenchmarkNewOperand  	 5000000	       373 ns/op
BenchmarkNewOperand-2	 5000000	       342 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/language	4.338s
?   	github.com/nicksnyder/go-i18n/i18n/language/codegen	[no test files]
PASS
BenchmarkExecuteNilTemplate         	1000000000	         2.64 ns/op
BenchmarkExecuteNilTemplate-2       	1000000000	         2.63 ns/op
BenchmarkExecuteHelloWorldTemplate  	1000000000	         2.63 ns/op
BenchmarkExecuteHelloWorldTemplate-2	1000000000	         2.62 ns/op
BenchmarkExecuteHelloNameTemplate   	 1000000	      1026 ns/op
BenchmarkExecuteHelloNameTemplate-2 	 2000000	       946 ns/op
BenchmarkSprintf                    	10000000	       203 ns/op
BenchmarkSprintf-2                  	10000000	       181 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/translation	19.727s
?   	github.com/nicksnyder/go-i18n/vendor/gopkg.in/yaml.v2	[no test files]
$ git checkout concurrent-bundle
Switched to branch 'concurrent-bundle'
$ go test ./... -bench=. -cpu=1,2
PASS
ok  	github.com/nicksnyder/go-i18n/goi18n	0.019s
PASS
ok  	github.com/nicksnyder/go-i18n/i18n	0.007s
PASS
BenchmarkTranslateNonPluralWithMap            	 1000000	      1335 ns/op
BenchmarkTranslateNonPluralWithMap-2          	 1000000	      1236 ns/op
BenchmarkTranslateNonPluralWithStruct         	 1000000	      1712 ns/op
BenchmarkTranslateNonPluralWithStruct-2       	 1000000	      1608 ns/op
BenchmarkTranslateNonPluralWithStructPointer  	 1000000	      1845 ns/op
BenchmarkTranslateNonPluralWithStructPointer-2	 1000000	      1806 ns/op
BenchmarkTranslatePluralWithMap               	 1000000	      1978 ns/op
BenchmarkTranslatePluralWithMap-2             	 1000000	      1652 ns/op
BenchmarkTranslatePluralWithMapAndCountField  	 1000000	      1683 ns/op
BenchmarkTranslatePluralWithMapAndCountField-2	 1000000	      1613 ns/op
BenchmarkTranslatePluralWithStruct            	 1000000	      2239 ns/op
BenchmarkTranslatePluralWithStruct-2          	 1000000	      2301 ns/op
BenchmarkTranslatePluralWithStructPointer     	 1000000	      2368 ns/op
BenchmarkTranslatePluralWithStructPointer-2   	  500000	      2317 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/bundle	24.832s
PASS
BenchmarkNewOperand  	 5000000	       386 ns/op
BenchmarkNewOperand-2	 5000000	       359 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/language	4.495s
?   	github.com/nicksnyder/go-i18n/i18n/language/codegen	[no test files]
PASS
BenchmarkExecuteNilTemplate         	1000000000	         2.88 ns/op
BenchmarkExecuteNilTemplate-2       	1000000000	         2.73 ns/op
BenchmarkExecuteHelloWorldTemplate  	1000000000	         2.67 ns/op
BenchmarkExecuteHelloWorldTemplate-2	1000000000	         2.81 ns/op
BenchmarkExecuteHelloNameTemplate   	 1000000	      1113 ns/op
BenchmarkExecuteHelloNameTemplate-2 	 1000000	      1041 ns/op
BenchmarkSprintf                    	10000000	       201 ns/op
BenchmarkSprintf-2                  	10000000	       203 ns/op
ok  	github.com/nicksnyder/go-i18n/i18n/translation	18.801s
?   	github.com/nicksnyder/go-i18n/vendor/gopkg.in/yaml.v2	[no test files]

@nicksnyder
Copy link
Owner

Thanks for the perf results. Here is the comparison using benchcmp. Some noticeable slowdown but in absolute terms it is still very small.

From @ArthurHlt 's benchmarks:

benchmark                                          old ns/op     new ns/op     delta
BenchmarkTranslateNonPluralWithMap                 2125          2092          -1.55%
BenchmarkTranslateNonPluralWithMap-2               1869          2196          +17.50%
BenchmarkTranslateNonPluralWithStruct              2548          2788          +9.42%
BenchmarkTranslateNonPluralWithStruct-2            2588          2803          +8.31%
BenchmarkTranslateNonPluralWithStructPointer       3049          3239          +6.23%
BenchmarkTranslateNonPluralWithStructPointer-2     3075          3253          +5.79%
BenchmarkTranslatePluralWithMap                    2367          3153          +33.21%
BenchmarkTranslatePluralWithMap-2                  2478          2934          +18.40%
BenchmarkTranslatePluralWithMapAndCountField       2545          2694          +5.85%
BenchmarkTranslatePluralWithMapAndCountField-2     2276          2929          +28.69%
BenchmarkTranslatePluralWithStruct                 3402          4251          +24.96%
BenchmarkTranslatePluralWithStruct-2               3358          3639          +8.37%
BenchmarkTranslatePluralWithStructPointer          3445          3734          +8.39%
BenchmarkTranslatePluralWithStructPointer-2        3443          3854          +11.94%
BenchmarkNewOperand                                445           453           +1.80%
BenchmarkNewOperand-2                              450           453           +0.67%
BenchmarkExecuteNilTemplate                        4.65          4.69          +0.86%
BenchmarkExecuteNilTemplate-2                      4.59          4.73          +3.05%
BenchmarkExecuteHelloWorldTemplate                 4.67          4.65          -0.43%
BenchmarkExecuteHelloWorldTemplate-2               4.63          4.70          +1.51%
BenchmarkExecuteHelloNameTemplate                  1528          1546          +1.18%
BenchmarkExecuteHelloNameTemplate-2                1590          1586          -0.25%
BenchmarkSprintf                                   224           224           +0.00%
BenchmarkSprintf-2                                 221           264           +19.46%

From @emosbaugh 's benchmarks

benchmark                                          old ns/op     new ns/op     delta
BenchmarkTranslateNonPluralWithMap                 1254          1335          +6.46%
BenchmarkTranslateNonPluralWithMap-2               1174          1236          +5.28%
BenchmarkTranslateNonPluralWithStruct              1660          1712          +3.13%
BenchmarkTranslateNonPluralWithStruct-2            1495          1608          +7.56%
BenchmarkTranslateNonPluralWithStructPointer       1731          1845          +6.59%
BenchmarkTranslateNonPluralWithStructPointer-2     1589          1806          +13.66%
BenchmarkTranslatePluralWithMap                    1670          1978          +18.44%
BenchmarkTranslatePluralWithMap-2                  1565          1652          +5.56%
BenchmarkTranslatePluralWithMapAndCountField       1542          1683          +9.14%
BenchmarkTranslatePluralWithMapAndCountField-2     1425          1613          +13.19%
BenchmarkTranslatePluralWithStruct                 2069          2239          +8.22%
BenchmarkTranslatePluralWithStruct-2               1959          2301          +17.46%
BenchmarkTranslatePluralWithStructPointer          2105          2368          +12.49%
BenchmarkTranslatePluralWithStructPointer-2        2000          2317          +15.85%
BenchmarkNewOperand                                373           386           +3.49%
BenchmarkNewOperand-2                              342           359           +4.97%
BenchmarkExecuteNilTemplate                        2.64          2.88          +9.09%
BenchmarkExecuteNilTemplate-2                      2.63          2.73          +3.80%
BenchmarkExecuteHelloWorldTemplate                 2.63          2.67          +1.52%
BenchmarkExecuteHelloWorldTemplate-2               2.62          2.81          +7.25%
BenchmarkExecuteHelloNameTemplate                  1026          1113          +8.48%
BenchmarkExecuteHelloNameTemplate-2                946           1041          +10.04%
BenchmarkSprintf                                   203           201           -0.99%
BenchmarkSprintf-2                                 181           203           +12.15%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@emosbaugh @nicksnyder @ArthurHlt