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

Optimizing /user/buy/x in 40-50% #14984

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Conversation

italojs
Copy link

@italojs italojs commented Oct 30, 2023

I'm experimenting with some performance monitoring tools on Habitica for my studies, so I found a little unoptimized code

Changes

1

Optimized website/common/script/libs/getOfficialPinnedItems.js > getOfficialPinnedItems function, we are using 2 optimizations:

  • I'm grouping the gears by Set, it will reduce the cost of filtering the items
  • using a JSON structure to get the set to be added, will avoid the filter usage

It reduced the endpoint time by ~40%

That values in my local environment, in prod this same endpoint is consuming ~200ms

Unptimized
Screenshot 2023-10-30 at 18 07 29

Optimized
Screenshot 2023-10-30 at 17 41 41

2

Added a flag to avoid using clustering, it helps when we are monitoring the application to tunning stuff


UUID: aa5a278e-675d-4d8f-ae2b-90391b2d7218

@italojs
Copy link
Author

italojs commented Nov 2, 2023

i will work on that failed check on the weekend

Copy link
Member

@negue negue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you found this issue - something we all missed I guess haha, just a few comments there

as for the CI issues - we had an issue with our package-lock.json that just automagically broke all installations hahah, so once you pull the latest develop it should(TM) work again

website/common/script/libs/getOfficialPinnedItems.js Outdated Show resolved Hide resolved
website/common/script/libs/getOfficialPinnedItems.js Outdated Show resolved Hide resolved
website/server/index.js Outdated Show resolved Hide resolved
@italojs
Copy link
Author

italojs commented Nov 4, 2023

@negue ready for review again
thanks for the develop test heads up, it saved me soooo much time

Copy link
Member

@negue negue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you also pull develop branch into yours? that way our CI's can then do their job 😬 and we can see if something else broke (you never know 😆 )

@@ -88,4 +88,5 @@
"REDIS_PORT": "1234",
"REDIS_PASSWORD": "12345678",
"TRUSTED_DOMAINS": "localhost,habitica.com"
"NO_CLUSTER": "false"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove all now not needed changes from the PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm sorry, i thought was removed

@italojs
Copy link
Author

italojs commented Nov 6, 2023

@negue the npm error in CI/CD continues, what we should do in this case?

@italojs
Copy link
Author

italojs commented Nov 6, 2023

note: the server.js -> server was because the linter didnt allow me to build the app without this change

@negue
Copy link
Member

negue commented Nov 9, 2023

@negue the npm error in CI/CD continues, what we should do in this case?

yes you need to pull our current develop state into your develop and then push it here - otherwise the old issue we had on develop won't vanish 😬

note: the server.js -> server was because the linter didnt allow me to build the app without this change

oh strange, well if the CI allows it afterwards I don't see a problem on it

@italojs
Copy link
Author

italojs commented Nov 11, 2023

@negue I rebased but the ci isn't running, must we trigger it?

@negue
Copy link
Member

negue commented Nov 11, 2023

@negue I rebased but the ci isn't running, must we trigger it?

Correct, the CI is not run for new contributors (and maybe all non members? I don't know what rule for that is) so we have to trigger them manually

Now whats only left is to have the tests running again 😁
https://github.com/HabitRPG/habitica/actions/runs/6825288001/job/18587356891?pr=14984#step:6:1

I think all other failed tests just have the same "issue"

@italojs
Copy link
Author

italojs commented Nov 13, 2023

ok, but why it is blocked for nonmembers? i mean, it makes the PR pretty long

support needed

well, i need support here, I guess the last broken tests isn't related to my changes but it is affecting it and it could be a bug

possible bug description

We have some hardcoded events in ...website/common/script/content/constants/events.js file, that events dont match with the current date, because this, the CURRENT_EVENT is coming empty in ./shops-seasonal.config file, that makes sense, we dont have any event for this month

Because this CURRENT_EVENT comes undefined
is it can be undefined? I mean, exists a "noEvent" property there that is being filtered

for me, it looks like a bug and it doesn't have a relation with these PR changes

my changes related to the possible bug

anyway, I improved the getOfficialPinnedItems validation for this case and the tests are passing for the "common" session now
Screenshot 2023-11-13 at 11 12 32

@italojs
Copy link
Author

italojs commented Nov 13, 2023

could you run the tests again, please?

@negue
Copy link
Member

negue commented Nov 17, 2023

Tests seems to work 🎉 apart from those two which seems to be broken for a while now (gonna take a look at these right now) - but your tests are fine!

Thank you for the work!

@SabreCat could you take a look at this PR and/or merge it? :)

@italojs
Copy link
Author

italojs commented Nov 26, 2023

@SabreCat

@italojs
Copy link
Author

italojs commented Dec 3, 2023

@negue do you know another admin to check this PR?
or have some updates about the tests?

@SabreCat
Copy link
Member

SabreCat commented Dec 4, 2023

Thanks for this, @italojs, and welcome to the ranks of the Blacksmiths at tier 1!
I'm going to put this on one of our testing servers for folks to do some exploratory QA, but we should be good to go! Thanks for bearing with us through the process here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants