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

Fix test cases. #1

Merged
merged 3 commits into from
Dec 23, 2014
Merged

Fix test cases. #1

merged 3 commits into from
Dec 23, 2014

Conversation

jridgewell
Copy link
Contributor

First, regions have a pointer to its el. No need for a lookup. Second, I'm
detaching all contents before running the replaces. Third, never replace
top view's el, just modify.

First, regions have a pointer to its el. No need for a lookup.
Second, I'm detaching all contents before running the replaces.
Third, never replace top view's el, just modify.
@jridgewell
Copy link
Contributor Author

With these changes:

Test 2

  • DOM ops: 10001
    Total time: 664.9459999753162
  • DOM ops: 10001
    Total time: 633.075000019744
  • DOM ops: 10001
    Total time: 658.1039999146014

Test 3

  • DOM ops: 2
    Total time: 608.6029999423772
  • DOM ops: 2
    Total time: 755.5380000267178
  • DOM ops: 2
    Total time: 669.0780000062659

@@ -26,11 +29,11 @@
// them with cloned versions of the elements in the existing
// tree. This causes 0 ops
$listClone.children().each(function(index, li) {
$(li).replaceWith(test.$el.find('.'+li.className).clone(true, true));
Copy link
Owner

Choose a reason for hiding this comment

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

I think we want the clone to deeply clone data and events, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, let me update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it defaults to the value of withDataAndEvents. 😃

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, would ya look at that!

@jamesplease
Copy link
Owner

Looking good, looking good. Just one Q, and I'll merge it in!

@jridgewell
Copy link
Contributor Author

Additionally, I was able to kick both test's asses using the same kind of placeholder that I used in regions. It's in test 4.

Test 4

  • DOM ops: 10004
    Total time: 420.87700008414686
  • DOM ops: 10004
    Total time: 389.65000002644956
  • DOM ops: 10004
    Total time: 406.3959999475628

It uses a placeholder, so we don't have to detach each child node, just
the parent.
@jamesplease
Copy link
Owner

Everything I've read suggests to me that there's got to be some test case we can find where the 10k+ DOM op methods will be incredibly slow...maybe mobile?

There's no way people would talk so much about slow reflows if these DOM operations were so quick.

...right?

jamesplease added a commit that referenced this pull request Dec 23, 2014
@jamesplease jamesplease merged commit 8236271 into jamesplease:master Dec 23, 2014
@jridgewell
Copy link
Contributor Author

I thought so too. I read http://korynunn.wordpress.com/2013/03/19/the-dom-isnt-slow-you-are/, and while it's a bit aggressive, it brings up one very good point:

Really? You don’t think it can be as fast? You think that your proficiency as a developer outstrips the devs working under the hood of chrome? firefox?

I think if you incrementally add/remove/replace like the original test 2 was doing, you're gonna have a slow time. But the DOM is fast, it's just not object property access fast.

@jamesplease
Copy link
Owner

I should clarify...whenever I refer to DOM ops, I really mean the ones that affect nodes that are children of the document. In other words, the ones that I'm counting in these tests.

I expect that the DOM, when detached, will function performance-wise on the same order of magnitude as VDom. The whole point of VDom, as I understand it, is to reduce the # of those DOM ops that could cause reflows. Yet these tests seem to be showing that my thinking on this may simply be completely wrong :)

I was also thinking that your tests would show an empty list for some time, but, it seems they're blinking exactly the same.

@jamesplease
Copy link
Owner

Checking out this article, I'm wondering if this may have something to do with it:

This means that if the changes happen quickly enough in the same thread, they may only produce one reflow. However, this cannot be relied on, especially considering the various different speeds of devices that Opera runs on.

It's probably worthwhile to test these on various devices, too.

@jridgewell
Copy link
Contributor Author

Probably. I don't think a reflow can happen until the event loop moves on?

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

2 participants