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

DOC Performance improvements for changelog #333

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Aug 23, 2023

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Note: I'm not doing a full review, this is just some questions I had in passing.

en/04_Changelogs/5.1.0.md Show resolved Hide resolved
- RelationList::foreignIDFilter()
- HasManyList::foreignIDFilter() - 108% faster (0.1584s vs 0.3304s)
- ManyManyList::foreignIDFilter() - 108% faster (0.1529s vs 0.3119s)
- ManyManyThroughList::foreignIDFilter() - 27% faster (0.6901s vs (0.8766s)
Copy link
Member

Choose a reason for hiding this comment

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

How did you test this? I'm guessing you didn't just call foreignIDFilter()?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've linked to the test setup in the description - silverstripe/silverstripe-framework#10904 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Might be worth being explicit about the test, something like "This was tested by passing {x number} IDs into the forForeignID() method of each list class, then executing the query"

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, I omitted the 'then executing the query' bit cos I'm "sharing" the text with byIDs() - however I don't think we need to be that pedantic here, I think the message of "made the method run faster" is clear

Copy link
Contributor

@maxime-rainville maxime-rainville left a comment

Choose a reason for hiding this comment

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

Maybe this is just me, but I'm not a big fan of the "X% faster" metric in this context. I think its difficult to understand what we are talking about.

I would rather say that the "response time has been reduced by X%".

@emteknetnz
Copy link
Member Author

As discussed offline, we'll keep the % faster language since it's more commonly used and easier to say and easiest to understand that it's a "good thing", though it's less "grammatically correct". Saying that query time has been reduced could be misinterpreted as being a "bad" thing since the word "reduced" is generally negative. Using the word "faster" make it more immediately obvious that it's positive.

e.g.

DataList::byIDs() - 198% faster - (0.0608s vs 0.1812s)

vs

DataList::byIDs() - query time has been reduced by 66% - (0.0608s vs 0.1812s)

@maxime-rainville maxime-rainville merged commit 61a9580 into silverstripe:5 Aug 31, 2023
@maxime-rainville maxime-rainville deleted the pulls/5/perf branch August 31, 2023 07:05
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.

None yet

3 participants