habitica icon indicating copy to clipboard operation
habitica copied to clipboard

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

Open italojs opened this issue 1 year ago • 13 comments

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 avatar Oct 30 '23 21:10 italojs

i will work on that failed check on the weekend

italojs avatar Nov 02 '23 11:11 italojs

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

italojs avatar Nov 04 '23 19:11 italojs

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

italojs avatar Nov 06 '23 09:11 italojs

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

italojs avatar Nov 06 '23 09:11 italojs

@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

negue avatar Nov 09 '23 20:11 negue

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

italojs avatar Nov 11 '23 09:11 italojs

@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"

negue avatar Nov 11 '23 13:11 negue

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 avatar Nov 13 '23 11:11 italojs

could you run the tests again, please?

italojs avatar Nov 13 '23 11:11 italojs

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? :)

negue avatar Nov 17 '23 00:11 negue

@SabreCat

italojs avatar Nov 26 '23 13:11 italojs

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

italojs avatar Dec 03 '23 17:12 italojs

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.

SabreCat avatar Dec 04 '23 23:12 SabreCat