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

Steamtracker fixes; version increment #54

Merged
merged 2 commits into from
May 28, 2021

Conversation

Luckz
Copy link
Contributor

@Luckz Luckz commented May 27, 2021

ST returns appid as int, which was breaking functionality because we were ===-comparing to a .toString().
I noticed we do a .find on an array here, which is potentially quite slow, so instead I changed it to one-time convert array to object for O(1) lookups and (hopefully 😆) added code to remove old storage if needed.

The dynamicstore-sourced lookups are done using .includes. I assume making a set or object out of all those arrays would make for faster lookups, but might be computationally expensive itself as many people poll a fresh dynamicstore on every single web request.

@Luckz
Copy link
Contributor Author

Luckz commented May 27, 2021

BTW, why do we have the setTimeout(() in line 348?

Luckz added 2 commits May 27, 2021 16:33
Both improves lookup performance and fixes a bug where we were looking for the app id as string when it was an integer.
@Revadike
Copy link
Owner

Revadike commented May 27, 2021

According to https://jsbench.me/3pkjlwzhbr/1 an object key lookup is indeed the fastest. I was thinking maybe Set would be equivalent in performance, but this is not the case.

@Revadike Revadike merged commit 22f1017 into Revadike:master May 28, 2021
@Luckz
Copy link
Contributor Author

Luckz commented May 28, 2021

(I was going by https://github.com/anvaka/set-vs-object myself.)
It doesn't really matter a lot how much faster a object lookup might be than a set considering in your example the set is already a thousand times (give or take) faster than array includes. The potential extra speedup might be meaningless.

To really squeeze out performance we'd need to see how much effort it is on each page load to

  • load object from GM storage vs load array from GM storage and convert to Set on the fly [since you can't store a Set in GM storage], then compare a humongously big page full of steam links to each
  • refresh dynamicstore/userdata and convert owned/wishlisted/ignored to either object or Set; in the case of Set, throwing away the ignore type and only keeping the app id «some people refresh dynamicstore/userdata on every page load, some only every few minutes», of course with an account that owns some 10k-25k subs

@Revadike
Copy link
Owner

I think its better to discuss this further on #14
Also, I made some changes as well, not sure if you saw it: 03abbf4

@Luckz Luckz mentioned this pull request May 28, 2021
@Revadike
Copy link
Owner

Revadike commented May 28, 2021

BTW, why do we have the setTimeout(() in line 348?

I thought I could delete it, but I was wrong. This is added to avoid having the page hang when loading, because it is waiting on our script execution. setTimeout essentially works as a new thread. I added it back.

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

Successfully merging this pull request may close these issues.

2 participants