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

Adding bottleneck vanilla test sites #803

Merged
merged 1 commit into from
Jan 12, 2017
Merged

Adding bottleneck vanilla test sites #803

merged 1 commit into from
Jan 12, 2017

Conversation

krisxw
Copy link
Contributor

@krisxw krisxw commented Jan 4, 2017

Adding some vanilla test sites in the react-server-test-pages package. These are very straightforward test sites, where the elements site loads 99,999 random elements from a set of predetermined elements, and the data requests site makes 999,999 data requests. Finally, the middleware site has a chain of 100 (identical) middleware that performs a bunch of no-ops on each. That site is currently blank, but there is work being done when it is accessed.

Some performance metrics are produced by default if one just looks in the browser console. Ideally we will integrate these with automated testing suites in the near future. I'm planning on sending out another PR for some other simple test sites in the near future, but wanted to get the ball rolling on these changes sooner rather than later.

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2017

CLA assistant check
All committers have signed the CLA.

@krisxw krisxw force-pushed the master branch 3 times, most recently from d50fcb2 to ec05c80 Compare January 5, 2017 23:00
Copy link
Member

@roblg roblg left a comment

Choose a reason for hiding this comment

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

Left some initial feedback.

Can we add a comment to each of these files indicating what it's testing? For example, what would a "failure" look like in the browser or server console?

}
return (<div>Data requests complete!</div>);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Technically I think we'd want the data requests to be kicked off in handleRoute. Practically, I'm not sure if that will make a difference.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, these should get kicked off in handleRoute, and then we should make root elements wait for them.

Ideally we could style the root elements so they all fit above the fold and affect the WPT speed index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not incorporate this feedback into my most recent commit. I'll address this in the next commit.


const colorThings = [];
for (var i = 0; i < 99999; i++) {
let selection = Math.floor(Math.random() * 7);
Copy link
Member

Choose a reason for hiding this comment

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

let selection = Math.floor(Math.random() * ColorWheel.length)

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can comment this if it's confusing to read. Basically, I wanted a site that served something a bit more interesting than a bunch of monochrome elements, so it serves a random assortment of various elements from the colors of the rainbow. A bit unnecessary maybe, but it was very little effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @roblg might be suggesting a replacement for the hard-coded 7 here.

I'll add that this can be a const. 👹

return (<div>
{colorThings}
</div>);
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to return a single element here with 100k children? Or 100k elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't appreciate the difference. I'm wanting 100k elements, so I'll adjust this accordingly.

JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware,
JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware,
JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware,
JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware, JsonResponseMiddleware];
Copy link
Contributor

Choose a reason for hiding this comment

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

_.range(80).map(() => JsonResponseMiddleware);

;)

export default class ElementsPage {

getElements() {

const colorThings = [];
for (var i = 0; i < 99999; i++) {
let selection = Math.floor(Math.random() * 7);
for (var i = 0; i < 10000; i++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After changing this behavior such that I was correctly returning a large number of elements (not one element with a large number of children), I found myself running out of heap space in javascript, so this number came down intentionally.

… and number of data requests. This includes visual functionality as elements are rendered and requests are fulfilled.
Copy link
Collaborator

@doug-wade doug-wade left a comment

Choose a reason for hiding this comment

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

Thanks @krisxw

@drewpc
Copy link
Collaborator

drewpc commented Jan 12, 2017

Looks good, thanks!

@drewpc drewpc merged commit 8eb18f4 into redfin:master Jan 12, 2017
@drewpc drewpc modified the milestone: 0.6.0 Jan 27, 2017
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.

6 participants