[Unicode]   Common Locale Data Repository : Bug Tracking Home | Site Map | Search
 
Modify

CLDR Ticket #11211(closed: fixed)

Opened 6 months ago

Last modified 2 weeks ago

Performance is slow when voting on multiple items on a page

Reported by: kristi Owned by: tbishop
Component: surveytool-other Data Locale:
Phase: dsub Review: mark
Weeks: Data Xpath:
Xref:

Description

Report from vetters is that the voting on multiple items on a page in rapid succession (after ~5 clicks). [
Current tool loses track and doesn’t respond.]

This was #2 in the Performance improvement goals in https://sites.google.com/site/cldr/index/cldr-engineer/sow

When this happens, the votes do not get saved and work has to be repeated.

Attachments

Change History

comment:1 Changed 6 months ago by kristi

  • Keywords STP1 added
  • Owner changed from anybody to tbishop
  • Priority changed from assess to critical
  • Status changed from new to accepted
  • Milestone changed from UNSCH to 34

comment:2 Changed 4 months ago by mark

This is amplified by the problem with focus. If the focus didn't change with a refresh, this would not be (quite) as bad.

comment:3 Changed 3 months ago by mark

That is, I think what is happening is that as fields are (asynchronously) validated, changes can cause the focus to switch to one of the past fields, thus disrupting the vetters work (and possibly causing errors).

comment:4 Changed 3 months ago by tbishop

Compare ticket 11312 "Make all id attributes unique for valid html in SurveyTool". The fact that id attributes are not unique may complicate accessing the right row when updating individual rows in response to user input.

comment:5 Changed 3 months ago by tbishop

When Survey Tool is having a hard time keeping up with a rapid sequence of votes, some rows temporarily get a light green background color, from a style in surveytool.css:

tr.tr_checking2 {
	background-color: #d4edd9;
	opacity: 0.75;
}

That style is used in survey.js:

function handleWiredClick(tr,theRow,vHash,box,button,what) {
...
	var loadHandler = function(json){
...
				if(json.submitResultRaw) { // if submitted..
					tr.className='tr_checking2';
					refreshRow2(tr,theRow,vHash,function(theRow) {

and also in a similar function handleCancelWiredClick.

comment:6 Changed 3 months ago by tbishop

The "loss of focus" is really two bugs, which have their own tickets, 11218 and 11265. While both of them are made worse by slow performance, their solutions don't really depend on speed of performance. A solution for 11218 is ready to commit. I'm working on 11265, which should have priority over this ticket 11211, since no matter how much we speed up the performance, slow network connections are always possible and it's simply a bug for the input window to disappear without good reason regardless of how long it takes to receive a response from the server. Still, some possible solutions to 11265 may happen to improve speed; generally we're regenerating the entire table every time there's an update, even when no more than a single row has changed data.

comment:7 Changed 3 months ago by tbishop

Actually, in survey.js, updateRow does get called by the loadHandler in refreshRow2, which is for updating just a single row, not the entire table. However, commonly when the little input window disappears, not only refreshRow2 but also updateStatus has been called, and the latter causes the entire table to be regenerated anyway.

comment:8 Changed 3 months ago by tbishop

  • Keywords performance added
  • type changed from unknown to surveytool
  • Component changed from unknown to survey
  • Milestone changed from 34 to 35

comment:9 Changed 2 months ago by tbishop

I've made a Selenium Webdriver script that simulates voting for several items in rapid succession, over and over. If I suspend the server at a random moment while that is happening, and there is a row with green background, most often the stack contains a thread similar to this:

Daemon Thread [/RefreshRow.jsp:en_CA:1316] (Suspended)	
	owns: SimpleTestCache  (id=183)	
	owns: CookieSession  (id=525)	
	owns: WebContext  (id=526)	
	owns: NioChannel  (id=527)	
	Collections$UnmodifiableCollection$1.hasNext() line: 1041	
	CollectionUtilities$PrefixIterator(CollectionUtilities$FilteredIterator).hasNext() line: 516	
	CheckDates.setCldrFileToCheck(CLDRFile, Options, List<CheckStatus>) line: 206	
	CheckCLDR$CompoundCheckCLDR.setCldrFileToCheck(CLDRFile, Options, List<CheckStatus>) line: 1281	
	TestCache$TestResultBundle.<init>(TestCache, CheckCLDR$Options) line: 26	
	SimpleTestCache.getBundle(CheckCLDR$Options) line: 98	
	STFactory$PerLocaleData.getTestResultData(CheckCLDR$Options) line: 1383	
	STFactory.getTestResult(CLDRLocale, CheckCLDR$Options) line: 1763	
	DataSection.make(PathHeader$PageId, WebContext, CookieSession, CLDRLocale, String, XPathMatcher, boolean, String) line: 2130	
	WebContext.getSection(String, XPathMatcher, String, WebContext$LoadingShow, PathHeader$PageId) line: 1587	
	WebContext.getSection(String, XPathMatcher, String, WebContext$LoadingShow) line: 1536	
	RefreshRow.jsp line: 142	
...
	TaskThread(Thread).run() line: 745

The call to getTestResult is usually there. This is consistent with our suspicion that running tests on candidate items is a major hot spot.

comment:10 Changed 2 months ago by tbishop

There's a new related ticket 11488 for the WebDriver test framework.

I've installed JVM Monitor http://www.jvmmonitor.org in Eclipse, and its CPU Hot Spot feature, like suspending the server at random, points to org.unicode.cldr.web.STFactory.getTestResult().

comment:11 Changed 2 months ago by tbishop

Given that getTestResult is a hot spot, one theory is that test caches aren't used as effectively as they could be. For example, they may sometimes get cleared when they don't need to be.

There was some doubt whether caches are shared between users. I've just learned that the same cache is at least sometimes shared between users. To do that, I created two nearly-identical users: both Adobe TC for "aa ar de fr". Then I logged into the Survey Tool running on localhost, with one user in Firefox, the other in Chrome. The Firefox user opens a page in a new locale (http://localhost:8080/cldr-apps/v#/de/Languages_A_D/), and the Java console shows:

Bundle null for Options:de/default/comprehensive/comprehensive/ in {SimpleTestCacheorg.unicode.cldr.test.SimpleTestCache@23b06032 Size: 0 ( 0/0}

That means that locale/coverage combination wasn't in the cache yet.

The Chrome user opens the same page, and the console shows:

Bundle refvalid: Options:de/default/comprehensive/comprehensive/ -> true

That means the combination was already in the cache.

Those console messages are from getBundle in SimpleTestCache.java.

Last edited 2 months ago by tbishop (previous) (diff)

comment:12 Changed 8 weeks ago by tbishop

As I wrote to the cldr-core list yesterday, it's not clear to me whether the current test-result caching avoids re-running tests.

SimpleTestCache caches instances of TestResultBundle, so those often don't need to be re-constructed. However, constructing a TestResultBundle doesn't populate it with test results. It's time-consuming to run the tests to populate the TestResultBundle. It looks as though each time a user visits a page, the tests get run for each path on that page, even though nothing has changed and we already have a valid TestResultBundle containing valid results. That TestResultBundle is used, but its existing results get replaced by new identical results.

It's very possible I'm mistaken; the code is complex. Otherwise, there appears to be potential for performance improvement by recognizing when a TestResultBundle already contains valid test results and re-using them.

comment:13 Changed 8 weeks ago by tbishop

Some of the objects involved are: TestResultBundle, SimpleTestCache, CheckCLDR, CheckStatus, DataSection.

TestResultBundle is an object bringing together per-locale test results. Each test result is a CheckStatus. This issue involves the relationship between TestResultBundle and CheckStatus.

In TestCache.java:

public abstract class TestCache implements XMLSource.Listener {
  public class TestResultBundle {
    protected List<CheckStatus> possibleProblems = new ArrayList<CheckStatus>();

TestResultBundle has a list of CheckStatus, named possibleProblems. Does that include ONLY results that are possible problems? If so, is there currently any way of remembering that a test has been run WITHOUT problems?

In DataSection.java:

  List<CheckStatus> item2Result = new ArrayList<CheckStatus>();
  checkCldr.check(xpath, item2Result, avalue);
  if (!item2Result.isEmpty()) {
    item2.setTests(item2Result);
  }

Questions:

  • Does test caching currently avoid re-running tests?
  • If it doesn't avoid re-running tests now, did it before? (Do we need to fix a broken cache feature, or implement a new cache feature?)
  • Is there currently a way of remembering (caching) that a test has been run WITHOUT problems?
  • What is the relation between possibleProblems and item2Result?

comment:14 Changed 8 weeks ago by tbishop

Notes from meeting with Mark:

possibleProblems may be limited to problems from initialization of the bundle, not from running tests.

OLD:

        public void check(String path, List<CheckStatus> result, String value) {
            cc.check(path, file.getFullXPath(path), value, options, result);
        }

PROPOSED NEW:

Option 1. Keep SimpleTestCache as it is, use simple map inside of TestResultBundle.

Option 2. Change the SimpleTestCache to be plain map, and use Cache in TestResultBundle

       Map<> pathCache<> = … // 

        public void check(String path, List<CheckStatus> result, String value) {
            cachedResult = pathCache.get(path);
            if (cachedResult != null) {
                result.addAll(cachedResult);
            } else {
               cc.check(path, file.getFullXPath(path), value, options, result);
               cachedResult = result.clone(); // maybe clone.
            }
            ...
        }

comment:15 Changed 8 weeks ago by tbishop

Tentative answers to the questions above:

  • Does test caching currently avoid re-running tests? NO
  • If it doesn't avoid re-running tests now, did it before? NO
  • Is there currently a way of remembering (caching) that a test has been run WITHOUT problems? NO
  • What is the relation between possibleProblems and item2Result? They're independent lists of results. We need to add a map or cache to TestResultBundle (options 1 and 2 above).

comment:16 Changed 7 weeks ago by tbishop

I've added this to TestResultBundle:

        private HashMap<String, List<CheckStatus>> pathCache;

along with code for storing and retrieving the results in pathCache.

I was going to run the unit tests and ConsoleCheckCLDR.java to see what difference pathCache makes, but it turns out TestCache and TestResultBundle aren't used except by Survey Tool. Even cldr-apps TestAll.java doesn't use them, except that when an STFactory is constructed these get created, but then not used in a meaningful way that I can see:

    TestCache gTestCache = new SimpleTestCache();
    TestCache gDiskTestCache = new SimpleTestCache();

Other than setFactory and getFactory, gTestCache and gDiskTestCache seem unused for TestAll.java. No TestResultBundle is ever constructed.

comment:17 Changed 7 weeks ago by tbishop

Running Survey Tool, pathCache appears to be put to good use, however we don't have a test to measure whether it actually saves time. With these println statements

        public void check(String path, List<CheckStatus> result, String value) {
            if (usePathCache) {
                List<CheckStatus> cachedResult = pathCache.get(path);
                if (cachedResult != null) {
                    result.addAll(cachedResult);
                    System.out.println("🔶 checking " + path + " value " + value + "; got from cache");
                    return;
                }
            }
            cc.check(path, file.getFullXPath(path), value, options, result);
            if (usePathCache) {
                pathCache.put(path, (List<CheckStatus>) result); // TODO: .clone()?
                System.out.println("🔷 checking " + path + " value " + value + "; added new to cache");
            }
        }

the console contains this the first time visiting http://localhost:8080/cldr-apps/v#/ar/Languages_A_D/

🔷 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value hy; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value ady; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value bin; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value البينية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value البينية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية (العالم); added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية الرسمية الحديثة; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية الرسمية الحديثة; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value agq; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value الأغم; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value الأغم; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="chn"] value الشينوك جارجون; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="chn"] value الشينوك جارجون; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="dyu"] value الدايلا; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="dyu"] value الدايلا; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="chb"] value التشيبشا; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="chb"] value التشيبشا; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value dar; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value الدارجوا; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value الدارجوا; got from cache
...

Interestingly, even the first time loading a page, the cache gets about twice as many hits as misses.

If we go to the next page, and then back to this page again, we get all hits, no misses:

🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value hy; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value ady; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; got from cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; got from cache
...

Note that there's no clone() in the new code -- we still need to check whether clone(), or some kind of deep or shallow copy of the test result is needed when it goes into the cache. My impression so far is that each CheckStatus item gets constructed newly for the particular test that's being run, and that the List is not re-used. For example, DataSection.java does this for each value to be tested:

            if (avalue != null && checkCldr != null) {
                List<CheckStatus> item2Result = new ArrayList<CheckStatus>();
                checkCldr.check(xpath, item2Result, avalue);

Since the List (in this case item2Result) is new each time, there should be no problem with adding it to the cache without cloning it -- it won't get changed later. However, CheckStatus does have many member objects, including "private CheckCLDR cause"; a deep copy of such members might ensure stability of cached CheckStatus objects, but maybe at a prohibitive cost in memory allocation.

comment:18 Changed 7 weeks ago by tbishop

cldr-apps TestAll.java never calls CheckCLDR.check().

cdlr-unittest TestAll.java does call CheckCLDR.check(), but only in one place, in TestCheckCLDR.java:

    public static void TestCheckConsistentCasing() {
        CheckConsistentCasing c = new CheckConsistentCasing(
            testInfo.getCldrFactory());
        Map<String, String> options = new LinkedHashMap<String, String>();
        List<CheckStatus> possibleErrors = new ArrayList<CheckStatus>();
        final CLDRFile english = testInfo.getEnglish();
        c.setCldrFileToCheck(english, options, possibleErrors);
        for (String path : english) {
            c.check(path, english.getFullXPath(path),
                english.getStringValue(path), options, possibleErrors);
        }
    }

That calls check() once for each path, all for the "en" locale. Complicating this bit of code with a TestResultBundle probably wouldn't do anything for performance, and wouldn't serve as a very meaningful unit test either.

ConsoleCheckCLDR.java does call CheckCLDR.check(), over and over, looping through many locales and paths. I think it does each locale/path combination only once, so the performance benefit of employing TestCache might not be very much; then again, we saw above that the cache gets many hits even on the first time loading a page so that might pay off for ConsoleCheckCLDR as well. What would it take to make ConsoleCheckCLDR employ TestCache?

comment:19 Changed 7 weeks ago by tbishop

I've just noticed that the pathCache implementation above isn't right yet. It doesn't take the value into account. The test result depends on the locale, path, and value. The bundle is already specific to a locale, so that's OK, but the key for pathCache needs to include the value.

That probably also explains why there were cache hits on the first pass: different candidate values for the same path.

comment:20 Changed 7 weeks ago by tbishop

I revised check() so that the key has the path and value concatenated. They're separated by a space though I'm not sure that's necessary; in theory "a" and "ab" might both occur as paths, and "b" and "bc" might both occur as values; then the key "abc" would be ambiguous, for either ("a" + "bc") or ("ab" + c). I'm supposing paths never end with a space.

        public void check(String path, List<CheckStatus> result, String value) {
            String key = path + " " + value;
            if (usePathCache) {
                List<CheckStatus> cachedResult = pathCache.get(key);
                if (cachedResult != null) {
                    result.addAll(cachedResult);
                    System.out.println("🔶 checking " + path + " value " + value + "; got from cache");
                    return;
                }
            }
            cc.check(path, file.getFullXPath(path), value, options, result);
            if (usePathCache) {
                pathCache.put(key, (List<CheckStatus>) result); // TODO: .clone()?
                System.out.println("🔷 checking " + path + " value " + value + "; added new to cache");
            }
        }

It turns out that we still get some hits the first time through, but there are only about half as many hits as misses:

🔷 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value hy; added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="hy"] value الأرمنية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="akk"] value الأكادية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value ady; added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ady"] value الأديغة; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value bin; added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value البينية; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="bin"] value البينية; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية (العالم); added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية الرسمية الحديثة; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="ar_001"] value العربية الرسمية الحديثة; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value agq; added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value الأغم; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="agq"] value الأغم; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="chn"] value الشينوك جارجون; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="chn"] value الشينوك جارجون; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="dyu"] value الدايلا; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="dyu"] value الدايلا; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="chb"] value التشيبشا; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="chb"] value التشيبشا; got from cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value dar; added new to cache
🔷 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value الدارجوا; added new to cache
🔶 checking //ldml/localeDisplayNames/languages/language[@type="dar"] value الدارجوا; got from cache

comment:21 follow-up: ↓ 24 Changed 7 weeks ago by tbishop

What if value is null? I don't know why, but sometimes check() is called like this:

    checkCldr.check(xpath, checkCldrResult, isExtraPath ? null : ourValue);

In Java, "path" + " " + null equals "path null". In case the string "null" is proposed as a candidate value, we'd better do something to distinguish null from "null".

I've handled that with special value "\n-NuL", used only in the pathCache key, in [14586].

Last edited 6 weeks ago by tbishop (previous) (diff)

comment:22 Changed 6 weeks ago by tbishop

I added a unit test for the new pathCache for TestResultBundle. Without pathCache, getting test results on a set of 19,698 paths takes 4.017 seconds the first time and 3.293 seconds the second time. With pathCache, it takes 4.125 seconds the first time and 0.046 seconds the second time.

comment:23 Changed 4 weeks ago by tbishop

This ticket is now identified with the TestCache changes to improve performance on the back end. Those changes are in branches/tbishop/t11211_fast_voting. I've asked approval to commit those changes to trunk, and if testing goes as expected, this ticket will be ready to close as fixed.

As agreed in meeting with Mark and Kristi, I've split off a new ticket: 11571 Avoid rebuilding entire table on Survey Tool update, which is about the front end and is completely separate from this ticket 11211 as far as code is concerned, though it addresses related performance and stability issues.

comment:24 in reply to: ↑ 21 Changed 4 weeks ago by srl

I've reviewed t11211_fast_voting and it's OK to merge with a comment below:

Replying to tbishop:

What if value is null? I don't know why, but sometimes check() is called like this:

    checkCldr.check(xpath, checkCldrResult, isExtraPath ? null : ourValue);

ourValue is already null in the isExtraPath case, probably an unnecessary check here. There's no possible value available because the data item does not exist (yet).

In Java, "path" + " " + null equals "path null". In case the string "null" is proposed as a candidate value, we'd better do something to distinguish null from "null".

I've handled that with special value "\n-NuL", used only in the pathCache key, in [14586].

This probably works fine, but it might be clearer to make the key be org.unicode.cldr.util.Pair<String, String>, the second value can be null and should be handled properly.

comment:25 Changed 4 weeks ago by tbishop

Per Steven's suggestion, I've switched to use org.unicode.cldr.util.Pair.

Change:

        private HashMap<String, List<CheckStatus>> pathCache;

to:

        private HashMap<Pair<String, String>, List<CheckStatus>> pathCache;

Change:

            pathCache = new HashMap<String, List<CheckStatus>>();

to:

            pathCache = new HashMap<Pair<String, String>, List<CheckStatus>>();

Change:

            String key = path + " " + ((value == null) ? "\n-NuL" : value);

to:

            Pair<String, String> key = new Pair<String, String>(path, value);

The unit test passes, and the results look the same in the browser. I'll go ahead and commit to trunk.

Last edited 4 weeks ago by tbishop (previous) (diff)

comment:26 Changed 4 weeks ago by tbishop

Committed to trunk with changeset [14650]. Will test on SmokeTest. No expected change in behavior other than faster response, as measured objectively by unit test and subjectively by interactive use in browser. Objective measurement of performance in browser will depend on work still in progress, see ticket 11488.

comment:27 Changed 4 weeks ago by tbishop

  • Status changed from accepted to reviewing
  • Review set to mark

Build succeeded and SmokeTest appears to be running OK.

comment:28 Changed 4 weeks ago by mark

For the future: the other way to do a cache is to have

Map<String, Map<String, List<CheckStatus>>

Slightly more complicated, since a put turns into:

submap = pathCache.get(path);
if (submap == null) {

pathCache.put(path, submap = new HashMap<>());

}
submap.put(value, list);
but you have to futz it it a bit for null.

comment:29 Changed 4 weeks ago by mark

Not saying you need to make this change: there are plusses & minuses to both approaches.

The advantage of a double-map is that the lookup avoids an object creation. The disadvantage is that it has two hashmap lookups.

comment:30 Changed 3 weeks ago by tbishop

Mark discovered a problem with phantom warnings in the Info Panel, that is, warnings for test failures that actually belonged to different values in different rows.

It turns out that TestResultBundle.check() calls CheckCLDR.check(), which calls result.clear() before it adds any results:

   public final CheckCLDR check(String path, String fullPath, String value, Options options,
        List<CheckStatus> result) {
...
        result.clear();
...

However, with the new pathCache as in revision [14650], when the test result is found in pathCache we don’t call CheckCLDR check(), and the list of results don’t get cleared so we get phantom warnings.

This can be fixed by calling result.clear() in TestResultBundle.check() before adding cached results.

In the process of studying this problem various other problems and potential improvements came to light. I've followed Mark's suggestions to use ConcurrentHashMap, and ImmutableList.copyOf, instead of HashMap for pathCache; make some members private, final; remove unused code. Also, create checkCldrResult and examplesResult lists newly for each row instead of clearing and re-using; merge SimpleTestCache into TestCache; and other minor clean-up and comments.

I’m making these changes on a new branch (branches/tbishop/t11211_checkCldrResult) for now. See revisions [14655], [14656], [14657].

comment:31 Changed 3 weeks ago by tbishop

Committed fixes to trunk with [14668].

comment:32 Changed 3 weeks ago by tbishop

Built OK. Tested on smoketest, looks OK:

http://cldr-smoke.unicode.org/smoketest/v#/chr/Smileys/257d9e3f66adac9

Info Panel has "Changes to this item require 4 votes.", no other warnings

http://cldr-smoke.unicode.org/smoketest/v#/chr/Smileys/6b83f9f0ecc10406

Info Panel has "Expected no more than 5 items(s), but was 6."

Need to set Comprehensive coverage to see all rows.

Last edited 3 weeks ago by tbishop (previous) (diff)

comment:33 Changed 2 weeks ago by mark

  • Status changed from reviewing to closed
  • Resolution set to fixed
View

Add a comment

Modify Ticket

Action
as closed
Next status will be 'new'
Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.