You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2021/03/09 15:34:12 UTC

[GitHub] [apisix-dashboard] bisakhmondal opened a new pull request #1569: feat: supports changing number of executing cpu cores for manager api

bisakhmondal opened a new pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [ ] Bugfix
   - [x] New feature provided
   - [ ] Improve performance
   - [ ] Backport patches
   
   - Related issues
   resolves #682 
   ___
   ### New feature or improvement
   
   Supports changing the number of os threads attached to the goroutines.
   
   For performance reason, if in the configuration file, the defined `max_cpu` is greater than the number of available cpu cores (including hyperthreading), the `manager api` will stick to the max available cores. As it has a certain limit to boost the performance since the context switching of goroutines from the same os thread is a lot faster than goroutines from different threads. 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#issuecomment-795208991


   @bisakhmondal  merged, thanks a lot.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] bisakhmondal commented on a change in pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#discussion_r590556229



##########
File path: api/internal/conf/conf.go
##########
@@ -249,3 +254,16 @@ func initEtcdConfig(conf Etcd) {
 		Prefix:    prefix,
 	}
 }
+
+// initialize parallelism settings
+func initParallelism(choiceCores int) {
+	if choiceCores < 0 {
+		return
+	}
+	maxSupportedCores := runtime.NumCPU()
+
+	runtime.GOMAXPROCS(maxSupportedCores)
+	if choiceCores < maxSupportedCores {
+		runtime.GOMAXPROCS(choiceCores)
+	}

Review comment:
       yeah, sure. Thanks for the review.
   
   Edit 1: One thing, I missed putting that the changes for making the default value of `GOMAXPROCS`  from 1 to `NumCPU` have been made on go version 1.5 ([thread](https://groups.google.com/g/golang-dev/c/POSw7qrelso/m/dI3YPTeGbkMJ)). Though in 1.13, manual resetting is not required, still I was trying to be extra sure. haha.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] bisakhmondal commented on a change in pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#discussion_r591057039



##########
File path: api/internal/conf/conf.go
##########
@@ -249,3 +254,16 @@ func initEtcdConfig(conf Etcd) {
 		Prefix:    prefix,
 	}
 }
+
+// initialize parallelism settings
+func initParallelism(choiceCores int) {
+	if choiceCores < 0 {

Review comment:
       True. It makes sense.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#discussion_r590911946



##########
File path: api/internal/conf/conf.go
##########
@@ -249,3 +254,16 @@ func initEtcdConfig(conf Etcd) {
 		Prefix:    prefix,
 	}
 }
+
+// initialize parallelism settings
+func initParallelism(choiceCores int) {
+	if choiceCores < 0 {

Review comment:
       how about
   ```suggestion
   	if choiceCores < 1 {
   ```
   
   because 0 means no limit.
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] codecov-io commented on pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#issuecomment-794937325


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=h1) Report
   > Merging [#1569](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=desc) (40dad6b) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/c8b235bb4a60c2d91840a4385649998bd1e9a841?el=desc) (c8b235b) will **increase** coverage by `4.05%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1569      +/-   ##
   ==========================================
   + Coverage   69.15%   73.21%   +4.05%     
   ==========================================
     Files          49       86      +37     
     Lines        3164     2322     -842     
     Branches        0      584     +584     
   ==========================================
   - Hits         2188     1700     -488     
   + Misses        728      622     -106     
   + Partials      248        0     -248     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | backend-e2e-test | `?` | |
   | backend-e2e-test-ginkgo | `?` | |
   | backend-unit-test | `?` | |
   | frontend-e2e-test | `73.21% <ø> (?)` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/log/log.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2xvZy9sb2cuZ28=) | | |
   | [api/internal/filter/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9hdXRoZW50aWNhdGlvbi5nbw==) | | |
   | [api/internal/filter/request\_id.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2ZpbHRlci9yZXF1ZXN0X2lkLmdv) | | |
   | [api/internal/core/store/validate.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvdmFsaWRhdGUuZ28=) | | |
   | [api/internal/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3JvdXRlLmdv) | | |
   | [api/internal/core/store/store\_mock.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmVfbW9jay5nbw==) | | |
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | | |
   | [...l/handler/route\_online\_debug/route\_online\_debug.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGVfb25saW5lX2RlYnVnL3JvdXRlX29ubGluZV9kZWJ1Zy5nbw==) | | |
   | [api/internal/core/entity/entity.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvZW50aXR5L2VudGl0eS5nbw==) | | |
   | [api/internal/handler/label/label.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvbGFiZWwvbGFiZWwuZ28=) | | |
   | ... and [124 more](https://codecov.io/gh/apache/apisix-dashboard/pull/1569/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=footer). Last update [c8b235b...40dad6b](https://codecov.io/gh/apache/apisix-dashboard/pull/1569?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
starsz commented on a change in pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#discussion_r590496205



##########
File path: api/internal/conf/conf.go
##########
@@ -249,3 +254,16 @@ func initEtcdConfig(conf Etcd) {
 		Prefix:    prefix,
 	}
 }
+
+// initialize parallelism settings
+func initParallelism(choiceCores int) {
+	if choiceCores < 0 {
+		return
+	}
+	maxSupportedCores := runtime.NumCPU()
+
+	runtime.GOMAXPROCS(maxSupportedCores)
+	if choiceCores < maxSupportedCores {
+		runtime.GOMAXPROCS(choiceCores)
+	}

Review comment:
       > GOMAXPROCS sets the maximum number of CPUs that can be executing simultaneously and returns the previous setting. It defaults to the value of runtime.NumCPU. If n < 1, it does not change the current setting. This call will go away when the scheduler improves.
   
   As the doc said, I think this will be more clear.
   
   ```golang
   maxSupportedCores := runtime.NumCPU()
   if choiceCores > maxSupportedCores {
         choiceCores = maxSupportedCores
   }
   
   runtime.GOMAXPROCS(choiceCores)
   ```
   




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] bisakhmondal commented on pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#issuecomment-795429313


   You're welcome. Thanks for the review guys.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] bisakhmondal commented on a change in pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
bisakhmondal commented on a change in pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569#discussion_r590556229



##########
File path: api/internal/conf/conf.go
##########
@@ -249,3 +254,16 @@ func initEtcdConfig(conf Etcd) {
 		Prefix:    prefix,
 	}
 }
+
+// initialize parallelism settings
+func initParallelism(choiceCores int) {
+	if choiceCores < 0 {
+		return
+	}
+	maxSupportedCores := runtime.NumCPU()
+
+	runtime.GOMAXPROCS(maxSupportedCores)
+	if choiceCores < maxSupportedCores {
+		runtime.GOMAXPROCS(choiceCores)
+	}

Review comment:
       yeah, sure. Thanks for the review.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [apisix-dashboard] starsz merged pull request #1569: feat: supports changing number of executing cpu cores for manager api

Posted by GitBox <gi...@apache.org>.
starsz merged pull request #1569:
URL: https://github.com/apache/apisix-dashboard/pull/1569


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org