Differential D7581
Speed up loading of diffs with a lot of unit test failures Authored by beales on Nov 13 2013, 7:54 PM. Tags None Referenced Files
Subscribers
Details
We've been having trouble with viewing diffs timing out when there's a lot of unit test failures. It was caused by formatting userdata for every single failure. The expensive part of this was actually creating the engine for every result, so moved the construction outside of the loop. Diffs that timed out (2 min) loading before load in around 6 seconds now. Loaded diffs that used to time out. Verified that details still looked right when Show Full Unit Test Results Is Clicked.
Diff Detail
Event TimelineComment Actions Particularly, differential.custom-remarkup-rules and differential.custom-remarkup-block-rules are gone in HEAD, so this should move away from newDifferentialMarkupEngine at a bare minimum. Comment Actions We could also consider cutting this off at, say, 100 results, as the data is already sorted and it seems unlikely that seeing more than 100 failures will ever be useful. This data is probably all going to move to Harbormaster at some point, but that won't be ready in the upstream for a while and probably won't be practical for Facebook for far longer. Comment Actions I'm pretty sure I've done as suggested in the other diff, but in my tests this is just as slow as no fix at all. Any idea if I'm doing something wrong? I'm working off of da6cdc6bd0f456d35333e73499301bc5d147aed7 because we aren't up to date on schema changes, just in case that could have something to do with this. Comment Actions
Oh, my suggestion above may not make much sense. I'm surprised this doesn't show a significant improvement against da6cdc6b (I didn't break the thing D7583 fixes until recently). Let me take a look. Comment Actions oops i diffed the wrong code here, "differential.diff" is actually "default", but makes no difference either way. Comment Actions Updated version of D7583 should fix the performance of MarkupOneOff. Rendering a page with 100 synthetic test failures dropped from 3.8s to 450ms for me locally with that plus this. If the test data contains expensive blocks (like dot {{{ ... }}} or codeblocks) hitting the rendering cache should have a big impact, but it won't impact things as much for mostly-text-with-some-links. Comment Actions Cool that sped stuff up. Thanks for the quick response. I'll clean this up and resubmit. Comment Actions Those are both in HEAD now. The rest of this looks good -- give me an updated version when you can and I think we're good to go. Thanks! Comment Actions Hey, I ran a little problem in testing. Some of our i18n tests are outputting special characters. Everything works okay on the first try (which populates the cache). However, on the second load, when the cache is hit, $blocks[$key]->getCacheData() is false, instead of a string, so we get "Argument 1 passed to PhutilRemarkupEngine::postprocessText() must be an instance of array, bool given" I've traced this down to the database layer (something below LiskDao). We try to insert a the string, but we only get the first part of it back. Then the unserialize call returns false which comes up all the way to where we throw the exception. Is this something you've seen before? Should I be handling i18n strings differently? I can fix this pretty easily by just skipping the cache for the entry when the cache brings back false (in PhabricatorMarkupEngine), but it seems like a pretty cheesy fix. Comment Actions Skipping the cache entry sounds good to me; we could also change cache_markupcache.cacheData to use a binary collation instead of utf8_bin. There's no risk of data in that column being truncated in the middle of a character, which is what we're generally protecting against by using the UTF8 collation. Comment Actions Yeah, looks exactly like that, except we end up with an exception instead of the truncated comment. I'll make it skip the entry. Thanks. Comment Actions Added the false check. Turned on preserve line breaks. Cleaned up the code. Should be solid now. |