Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Consider 2,000 lines to be a large file for hints. #5400

Merged
merged 4 commits into from
Nov 1, 2013

Conversation

dangoor
Copy link
Contributor

@dangoor dangoor commented Oct 2, 2013

This is a fix for #4922... only part of Document.js was being processed
because it was considered a "large file" at 250 lines. 250 lines seems
a bit too conservative. We'll want to see if there are complaints about
memory usage and I think we'll also want to experiment a bit with how
we manage Tern.

But, it seems to me that a file the size of Document.js should not be
getting truncated hints.

This is a fix for #4922... only part of Document.js was being processed
because it was considered a "large file" at 250 lines. 250 lines seems
a bit too conservative. We'll want to see if there are complaints about
memory usage and I think we'll also want to experiment a bit with how
we manage Tern.

But, it seems to me that a file the size of Document.js should not be
getting truncated hints.
@dangoor
Copy link
Contributor Author

dangoor commented Oct 3, 2013

Oops. Simple change that fixes the problem, but I forgot to run the tests. There are some failures to fix.

@dangoor
Copy link
Contributor Author

dangoor commented Oct 3, 2013

The unit tests were failing because one of the sample files had been padded to be above 250 lines (making it a "long file" by the old definition, thus triggering the partial update logic).

This seemingly minor change actually worries me now. I padded the file to be 2,000 lines long, triggering the new long file limit. The tests ran a lot slower. Additionally, I closed Brackets, relaunched and ran the code hinting tests. brackets-helper grew to nearly 500MB in memory usage and did not drop. I stashed my changes and went back to the 250 line version of the file. brackets-helper only grew to 215MB. Note that the only difference between the old file and the new is 1750 more lines of comment (no additional JS that Tern would be working with).

I don't know if this memory leak is due to the way the tests are written or if it's due to the way the code hinting extension is written.

@dangoor
Copy link
Contributor Author

dangoor commented Oct 3, 2013

Some testing and profiling later... it appears most likely that the issue I'm seeing is with the tests and not with the original change. Most of the CPU and memory in the running of the tests seems to be going into the test editor instances.

I did a test where I opened a fresh Brackets up with a project that contained only Document.js. With this change, memory usage was about 3MB more than before this change, which does not seem completely out of line given the extra data we're tracking by looking the whole file rather than just 250 lines.

I'm going to try to make the tests faster and less memory hungry.

@dangoor
Copy link
Contributor Author

dangoor commented Oct 24, 2013

I left the test file (file1.js) at the same size that it was previously. When doing that, there were a small number of tests that failed purely for timing reasons (run individually, they work fine... run in the suite, they do not). These tests can be fairly unpredictable because the code is designed to make sure that results are returned quickly.

I think there are some things we can do that will make the behavior faster and more predictable.

In the meantime, I think it's safe to set this limit to 2000 lines. We might consider waiting until the beginning of next sprint, though.

@jasonsanjose
Copy link
Member

Filed #5811 to capture re-enabling the tests. Merging.

jasonsanjose added a commit that referenced this pull request Nov 1, 2013
…sing

Consider 2,000 lines to be a large file for hints.
@jasonsanjose jasonsanjose merged commit fce09d4 into master Nov 1, 2013
@jasonsanjose jasonsanjose deleted the dangoor/4922-document-methods-missing branch November 1, 2013 18:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants