You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/02/01 10:36:24 UTC
[GitHub] [trafficserver] shukitchan opened a new pull request #7465: Select lua context per thread
shukitchan opened a new pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465
This should reduce chances for thread to use the same lua context to handle requests.
And that should help to lower cpu load
----------------------------------------------------------------
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] [trafficserver] traeak edited a comment on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779372959
before i think transactions would just grab the next context on the list. So lua script context usage would cycle through the whole list regardless of threads.
With the new configuration each thread grabs a context and holds onto it. Unless the pool size is a multiple of the transaction threads the configuration could be imbalanced wrt available resources per thread.
So instead of configuring a global pool then the configuration might be better as number of threads that share a context (with default being no sharing).
The reason why you would want context sharing is to save memory, otherwise lua may cause ats to crash (the reason why I did the "lua states" PR in the first place).
With the removal of the 2gb limitation in luajit 210b3 doing one context per thread and removing the context array entirely would do the job I think.
----------------------------------------------------------------
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] [trafficserver] shinrich commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shinrich commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-777833056
Looks good to me
----------------------------------------------------------------
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] [trafficserver] shukitchan commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-770755623
Still testing in production environment
----------------------------------------------------------------
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] [trafficserver] shukitchan commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-770756159
This change is actually suggested by @shinrich and she has been testing an earlier version of this fix.
----------------------------------------------------------------
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] [trafficserver] shukitchan commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779332003
It may be useful to have less context than thread for limiting the memory used.
I don't think it makes sense to have more context than thread.
So that's why i still have the configuration for the total number of context.
----------------------------------------------------------------
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] [trafficserver] zwoop commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
zwoop commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-783516050
@shukitchan I'm ok with this for 9.1.x, but unless you feel super strongly, I don't think we should get this in for 9.0.x (it's a performance optimization as I understand this).
----------------------------------------------------------------
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] [trafficserver] shukitchan commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-776270195
I have tested this out in our production environment. It looks good.
----------------------------------------------------------------
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] [trafficserver] traeak edited a comment on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779372959
before i think transactions would just grab the next context on the list. So lua script context usage would cycle through the whole list regardless of threads.
With the new configuration each thread grabs a context and holds onto it. Unless the pool size is a multiple of the transaction threads the configuration could be imbalanced wrt available resources per thread.
So instead of configuring a global pool then the configuration might be better as number of threads that share a context (with default being no sharing).
The reason why you would want context sharing is to save memory, otherwise lua may cause ats to crash (the reason why I did the "lua states" PR in the first place).
----------------------------------------------------------------
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] [trafficserver] bryancall commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
bryancall commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-777834241
@traeak is going to look at it.
----------------------------------------------------------------
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] [trafficserver] traeak commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
traeak commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779372959
before i think transactions would just grab the next context on the list. So lua script context usage would cycle through the whole list regardless of cpus
With the new configuration each thread grabs a context and holds onto it.
So instead of configuring a global pool then the configuration might be better applied to the number of per thread lua context pools (defaulting to one context per thread, technically one for remap and one for global).
----------------------------------------------------------------
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] [trafficserver] SolidWallOfCode commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-777834581
Probably OK, but I think it should use `thread_local` ([documentation](https://en.cppreference.com/w/cpp/language/storage_duration)). That is potentially faster and avoids the key management logic.
----------------------------------------------------------------
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] [trafficserver] traeak commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
traeak commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779323213
We might want to change how the lua states are configured since a context ends up being sticky per thread. Perhaps we should change things so that we configure number of threads per context (default to 1 thread per context).
----------------------------------------------------------------
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] [trafficserver] shukitchan merged pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan merged pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465
----------------------------------------------------------------
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] [trafficserver] shukitchan commented on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
shukitchan commented on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-778538713
lua plugin is C, not C++
----------------------------------------------------------------
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] [trafficserver] zwoop edited a comment on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
zwoop edited a comment on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-783516050
@shukitchan I'm ok with this for 9.1.x, but unless you feel super strongly, I don't think we should get this in for 9.0.x (it's a performance optimization as I understand this). If you disagree, and this is more than just a performance optimization, please dd the Project 9.0.x again.
----------------------------------------------------------------
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] [trafficserver] traeak edited a comment on pull request #7465: Select lua context per thread
Posted by GitBox <gi...@apache.org>.
traeak edited a comment on pull request #7465:
URL: https://github.com/apache/trafficserver/pull/7465#issuecomment-779372959
before i think transactions would just grab the next context on the list. So lua script context usage would cycle through the whole list regardless of threads.
With the new configuration each thread grabs a context and holds onto it. Unless the pool size is a multiple of the transaction threads the configuration could be imbalanced wrt available resources per thread.
So instead of configuring a global pool then the configuration might be better as number of threads that share a context (with default being no sharing).
The reason why you would want context sharing is to save memory, otherwise lua may cause ats to crash (the reason why I did the "lua states" PR in the first place).
With the removal of the 2gb limitation in luajit 210b3 doing one context per thread should work fine (and not blow up ATS).
----------------------------------------------------------------
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