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.

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

lower performance compared to tc_aws #114

Closed
mello7tre opened this issue Feb 23, 2023 · 6 comments · May be fixed by #138
Closed

lower performance compared to tc_aws #114

mello7tre opened this issue Feb 23, 2023 · 6 comments · May be fixed by #138
Labels

Comments

@mello7tre
Copy link

mello7tre commented Feb 23, 2023

Hi, i switched from tc_aws [7.0.2] to thumbor-aws [0.6.0], but i had to revert cause response time increased by ~ 200 ms.
My configuration is very simple and i use loader only (Bucket is get from url).

thumbor-aws conf:

AWS_LOADER_REGION_NAME = "eu-west-1"                                      
AWS_LOADER_BUCKET_NAME = None                                                          
AWS_LOADER_ROOT_PATH = ""                                                              
AWS_STORAGE_REGION_NAME = ""                                                           
AWS_STORAGE_S3_SECRET_ACCESS_KEY = ""                                                  
AWS_STORAGE_S3_ACCESS_KEY_ID = ""                                                      
AWS_STORAGE_S3_ENDPOINT_URL = ""

tc_aws conf:

TC_AWS_REGION = "eu-west-1"
TC_AWS_ENABLE_HTTP_LOADER = True

Could be probably related to the improvements made on tc_aws by @amanagr amanagr/aws#1 ?
(For whole PR thread: thumbor-community/aws#147).
I will try to investigate further and let you know if i discover something relevant.

Regards, Alberto.

@mello7tre
Copy link
Author

I made further tests and tried to use same approach of tc_aws to re-use s3_client.
I changed s3_client S3Client class to:

class S3Client:
    _client = None
    _instances = {}

    @staticmethod
    def __new__(cls, context):
        key = (context.config.AWS_LOADER_BUCKET_NAME, context.config.AWS_LOADER_REGION_NAME, context.config.AWS_LOADER_S3_ENDPOINT_URL)

        if not cls._instances.get(key):
            cls._instances[key] = super(S3Client, cls).__new__(cls)

        return cls._instances[key]

and checked if self._client does exist before creating a new one.

As a result performance are now comparable to the tc_aws one!

Mine was only a test, but i can confirm you that the difference in response time is caused by the "multiple" s3 client initialization compared to initialize it just one time (for bucket, region, endpoint).

@mello7tre
Copy link
Author

That fix work only if you do not put code in an async with context and so do not use aiobotocore > 0.12.

Just for info the PR #19, at the end re-use only the session not the client, and the bigger effort is on creating the whole s3 client.

Currently we have a fast tc_aws but that to re-use aiobotocore s3 client need to use an old (very) version.
And an up to date thumbor-aws that is not usable in production, compared to tc_aws, because increase reponse time of 0.2 sec (and under an high number of request, also memory usage).

@pfcodes
Copy link

pfcodes commented Mar 8, 2023

Thanks for this heads up

@mello7tre
Copy link
Author

mello7tre commented Mar 8, 2023

just for info i have mixed tc_aws solutions amanagr/aws#1 and thumbor-community/aws@b5058e6 in thumbor-aws
(for the second one i had to put _client = None in class definition root and not in class __init__)

This way i have thumbor-aws that use a persistent s3 client and that work using an up to date aiobotocore [2.x].
(i have it running in production without any problem so far)

@github-actions
Copy link

github-actions bot commented Apr 7, 2023

This issue is stale because it has been open 30 days with no activity. Remove the stale label or add a comment, or this issue will be closed in 5 days. You can always re-open if you still feel this is still an issue. Tag @heynemann for more information.

@github-actions github-actions bot added the Stale label Apr 7, 2023
@github-actions
Copy link

This issue was closed because it has been stale for 5 days with no activity.

lukaswelte added a commit to lukaswelte/thumbor-aws that referenced this issue Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants