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

CLDR Ticket #9566(closed: fixed)

Opened 3 years ago

Last modified 3 months ago

ST: STFactory startup optimization

Reported by: srl Owned by: tbishop
Component: surveytool-other Data Locale:
Phase: final Review: srl
Weeks: Data Xpath:
Xref:

ticket:9512

Description

(this is not just server startup, but 'loading a locale')

in loadVoteValues(), internalSetVoteForValue() is repeatedly called with each vote in the DB.

internalSetVoteForValue() internally (1) populates the PerXPathData and (2) calls setValueFromResolver to resolve and record the voting result as it happens, even though all of the votes aren't in yet.

However ,at startup time, we don't need to (could defer) actually counting the votes. In fact, loadVoteValues ignores the return from internalSetVoteForValue.

An optimization on loadVoteValues would be to:
(1) populate the PerXPathData (internalSetVoteForValue but without setValueFromResolver)
(2) for ( String path : PerXPathData.keySet()) { setValueFromResolver(path, …); } to now count/record all votes.

Otherwise, votes are needlessly counted before all are in.

Furthermore, if getResolverInternal needs to recurse (as per ticket:9512) it can't reliably do so unless it knows all the votes are in.

Attachments

Change History

comment:1 Changed 3 years ago by srl

a workaround in the aforementioned ticket in r12710 put in the RE_RESOLVE_ALL_XPATHS switch in STFactory. Turn this off (and remove the relevant code) when this is fixed.

comment:2 Changed 3 years ago by mark

  • Owner changed from anybody to srl
  • Priority changed from assess to critical
  • type changed from unknown to survey
  • Status changed from new to accepted
  • Milestone changed from UNSCH to 31

comment:3 Changed 23 months ago by srl

  • Milestone changed from 31 to 32

comment:4 Changed 16 months ago by srl

  • Milestone changed from 32 to 33

comment:5 Changed 14 months ago by srl

  • Owner changed from srl to backend

comment:6 Changed 11 months ago by mark

  • Milestone changed from 33 to 34

comment:7 Changed 10 months ago by kristi

  • Keywords Performance added

comment:8 Changed 10 months ago by kristi

Performance for first time load of a locale

comment:9 Changed 4 months ago by pedberg

  • Milestone changed from 34 to 35

comment:10 Changed 4 months ago by tbishop

  • Owner changed from backend to tbishop
  • Component changed from unknown to survey

comment:11 Changed 4 months ago by tbishop

Per today's meeting I've reassigned this to myself.

comment:12 Changed 3 months ago by tbishop

loadVoteValues is called only by makeSource.

makeSource is called from many other functions, including the constructor for PerLocaleData.

During a run of TestAll.java in cldr-apps, the first call to loadVoteValues has this stack trace:

STFactory$PerLocaleData.loadVoteValues(STFactory$DataBackedSource) line: 698
STFactory$PerLocaleData.makeSource(boolean) line: 1124
STFactory$PerLocaleData.getFile(boolean) line: 837
STFactory.handleMake(String, boolean, CLDRFile$DraftStatus) line: 1878
STFactory(Factory).makeResolvingSource(String, CLDRFile$DraftStatus) line: 162
STFactory.access$5(STFactory, String, CLDRFile$DraftStatus) line: 1
STFactory$PerLocaleData.makeSource(boolean) line: 1116
STFactory$PerLocaleData.getFile(boolean) line: 829
STFactory.get(CLDRLocale) line: 1781
STFactory.get(String) line: 1790
STFactory.handleMake(String, boolean, CLDRFile$DraftStatus) line: 1878
STFactory(Factory).make(String, boolean, CLDRFile$DraftStatus) line: 107
STFactory(Factory).make(String, boolean) line: 116
STFactory.make(CLDRLocale, boolean) line: 1882
TestSTFactory.TestBasicFactory() line: 57
NativeMethodAccessorImpl.invoke0(Method, Object, Object[]) line: not available [native method]
NativeMethodAccessorImpl.invoke(Object, Object[]) line: 62
DelegatingMethodAccessorImpl.invoke(Object, Object[]) line: 43
Method.invoke(Object, Object...) line: 498
TestFmwk$MethodTarget.execute() line: 397
TestFmwk$MethodTarget(TestFmwk$Target).run() line: 339
TestFmwk$ClassTarget.execute() line: 451
TestFmwk$ClassTarget(TestFmwk$Target).run() line: 339
TestFmwk$ClassTarget.execute() line: 451
TestFmwk$ClassTarget(TestFmwk$Target).run() line: 339
TestAll(TestFmwk).runTests(TestFmwk$TestParams, String[]) line: 614
TestAll(TestFmwk).run(String[], PrintWriter) line: 557
TestAll(TestFmwk).run(String[]) line: 526
TestAll.main(String[]) line: 88

comment:13 Changed 3 months ago by tbishop

Since we're primarily interested in "startup", meaning when we choose a new locale in the Survey Tool interface, I'm measuring the time spent in internalSetVoteForValue, by inserting some debugging code around that call in loadVoteValues, adding the time for each call. Some results on localhost are:

loadVoteValues called internalSetVoteForValue, locale = aa, took 0.317 seconds for 8 items
loadVoteValues called internalSetVoteForValue, locale = ar, took 2.383 seconds for 18425 items
loadVoteValues called internalSetVoteForValue, locale = fr, took 2.23 seconds for 25268 items
loadVoteValues called internalSetVoteForValue, locale = de, took 1.728 seconds for 19322 items

When I quit Eclipse and restart, I get somewhat different results each time. Another run gave:

loadVoteValues called internalSetVoteForValue, locale = aa, took 0.261 seconds for 8 items
loadVoteValues called internalSetVoteForValue, locale = ar, took 2.851 seconds for 18425 items
loadVoteValues called internalSetVoteForValue, locale = fr, took 4.409 seconds for 25268 items
loadVoteValues called internalSetVoteForValue, locale = de, took 1.18 seconds for 19322 items

Probably more consistent measurements could be gotten using a test that doesn't run on the server. Our usual tests, however, load a temporary db without thousands of votes. Anyway, I'll try making the suggested optimization and maybe it will be lot faster.

comment:14 Changed 3 months ago by tbishop

Here's the current internalSetVoteForValue:

        private final VoteResolver<String> internalSetVoteForValue(User user, String distinguishingXpath, String value,
            VoteResolver<String> resolver, DataBackedSource source, Integer voteOverride, Date when) throws InvalidXPathException {
            // Don't allow illegal xpaths to be set.
            if (!getPathsForFile().contains(distinguishingXpath)) {
                throw new InvalidXPathException(distinguishingXpath);
            }
            getXPathData(distinguishingXpath).setVoteForValue(user, distinguishingXpath, value, voteOverride, when);
            stamp.next();
            return resolver = source.setValueFromResolver(distinguishingXpath, resolver);
        }

Experimentally, instead of calling it from loadVoteValues, I copied all but the last line and pasted those lines into loadVoteValues like this:

                            double deltaTime = System.currentTimeMillis();
                            if (true) {
                                if (!getPathsForFile().contains(xpath)) {
                                    throw new InvalidXPathException(xpath);
                                }
                                getXPathData(xpath).setVoteForValue(theSubmitter, xpath, value, voteOverride, lastmod);
                                stamp.next();
                            } else {
                                internalSetVoteForValue(theSubmitter, xpath, value, resolver, dataBackedSource, voteOverride,
                                    lastmod); // last_mod                                
                            }
                            deltaTime = System.currentTimeMillis() - deltaTime;
                            internalSetVoteForValueTime += deltaTime;

The times change as follows:

loadVoteValues called (not) internalSetVoteForValue, locale = aa, took 0.001 seconds for 8 items
loadVoteValues called (not) internalSetVoteForValue, locale = ar, took 0.045 seconds for 18425 items
loadVoteValues called (not) internalSetVoteForValue, locale = fr, took 0.089 seconds for 25268 items
loadVoteValues called (not) internalSetVoteForValue, locale = de, took 0.022 seconds for 19322 items

Those are spectacularly shorter times, so the optimization appears worthwhile, provided of course that the code still does what it needs to. After its main loop, loadVoteValues has this:

                if (RE_RESOLVE_ALL_XPATHS) {
                    et = (SurveyLog.DEBUG) ? new ElapsedTimer("Re-resolver loading for xpaths in " + locale) : null;
                    int j = 0;
                    for (String xp : allPXDPaths()) {
                        resolver = dataBackedSource.setValueFromResolver(xp, resolver);
                        j++;
                    }
                    SurveyLog.debug(et + " - Re-resolver  - resolved " + j + " additional items, " + n + " total.");
                }

Does calling setValueFromResolver for each xpath at the end like this already accomplish what is needed?

comment:15 Changed 3 months ago by tbishop

Yes, the block for RE_RESOLVE_ALL_XPATHS already does the second part of what Steven suggested. The constants RESOLVE_ALL_XPATHS and RE_RESOLVE_ALL_XPATHS can be removed, effectively making them permanently false and true, respectively.

comment:16 Changed 3 months ago by tbishop

Testing on localhost looks good. Trying various locales and sublocales, existing votes are displayed, and I'm able to vote normally.

comment:17 Changed 3 months ago by tbishop

I've committed changeset [14537] to trunk. Will test on SmokeTest.

comment:18 Changed 3 months ago by tbishop

SmokeTest looks good. I logged in and went to several locales and sublocales. All show some existing votes, and when I add a new vote it shows along with the old votes.

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

comment:19 Changed 3 months ago by tbishop

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

comment:20 Changed 3 months ago by pedberg

  • Milestone changed from 35 to 34

Commits for 34, so milestone moves back to 34.

comment:21 Changed 3 months ago by pedberg

  • Phase changed from dsub to final

comment:22 Changed 3 months ago by srl

  • 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.