FWD » Core Development » Conversion Tools

Greg, would it be OK if I included the fix mentioned in #7143-1994 in 11782a?

Yes

Committed the fix documented in #7143-1994 in 11782a/16750.

I've committed the changes discussed in #11818-66 in 11782a/16751. Now we emit the warnings for missing multiThreadedAccess flag only at the most verbose debug level.

Greg, please review the changes.

Greg, please assess. I would like to proceed with testing.

Code Review Task Branch 11782a Revisions 16749 through 16751

In PatternEngine, the OPT_NUM_THREADS should not include the =. Doing so means that any whitepace added before the = will break the option.

Otherwise I'm fine with the changes.

I'm OK with the plan.

I've tested both PatternEngine.main and SearchTrees.main by running standalone commands on Hotel_GUI. Multi-threaded mode is now active, and there are no output differences compared to single-threaded execution.

I also tested ReportApi.runSearch() by executing searches in FWD Analytics. Now multi-threaded search is active, and report_server.log confirms it.

I ran full conversion regression testing on Hotel ChUI, Hotel GUI, ChUI regression testing app, OO app, and ETF, with no differences in the generated output.

Is any additional testing required for this task?

Rebased branch 11782a with trunk/16804. The new revision is 16808.

If we remove the addAstSpec call and determine the file count directly based on directory and fileSpec, the import will work, but we still get:
[java] Utilizing 1 concurrent thread(s) to process 0 file(s); 16 CPU(s), 1024 MiB heap.
This is confusing because the processed AST count will ultimately be 1. This happens because jar mode is still turned off.

I think a solution for this would be to load the workers only. This would turn jar mode on, and the count will be calculated properly.

Greg, Constantin, do you think we should go ahead with the implementation described in the previous note, so that ant import.db works again?

The alternative I had in mind was to turn jar mode on while counting the files, when the profile to be processed is schema/import. I no longer think that holds up. The engine would have to recognise the profile by name before loading it, which is the kind of special case we want to avoid, and jar mode has no off switch, so enabling it speculatively would leak into the rest of the run. Loading the workers first gets to the same place without the engine knowing anything about a particular profile, because the profile's own worker is what makes the decision.

The underlying issue is that the way the ASTs are looked for differs from one run to another. It depends on the profile, and the profile is loaded after the point where we need the count.

I'm sorry, I don't understand to root cause well enough to evaluate the options. Please provide more detail on the problem and on your recommended solution.

Greg, here is the problem in more detail, then the fix I now recommend.

The reason why 11782a resolves the file set early is that each rule variable gets its thread scope (shared, or one copy per worker thread) while the profile is being loaded. ConfigLoader.variable() asks PatternEngine.isSingleThreadedMode() at that moment. So the worker pool must be started, and its size decided, before the profile is loaded. The default size depends on the number of files to process. To have that number in time, 11782a added this call to main(), before engine.run(profile):

else if (directory != null && !callGraphMode)
{
   engine.addAstSpec(directory, fileSpec, true);
}
...
startThreads(numThreads, countAstSpecArtifacts(engine));

addAstSpec() does two things: it looks the files up, and it registers the result as the list the run will process.

The lookup gives a different answer at that point because the same directory / fileSpec pair resolves differently depending on whether "jar mode" is on:

• Jar mode off: Configuration.toFile() makes the path absolute against the project root and the file system is scanned.
• Jar mode on: the path stays relative and is resolved in the conversion folder, the way the application jar stores it.

Jar mode is turned on by the profile, not by the command line. ImportWorker's constructor calls ArtifactManager.setJarMode(), and that constructor runs when the profile is loaded, because schema/import declares the worker.

ant import.db runs PatternEngine ... schema/import data hotel.p2o. In the two cases that becomes:

The failure is silent because, before 11782a, the lookup happened only in initialize(), right after the profile is loaded:

loader.load(profile);                      // ImportWorker turns jar mode on here
if (directory != null && context.get().astSpecs.isEmpty())
{
   addAstSpec(directory, fileSpec, true);  // resolves ./cvt/data/hotel.p2o
}

With 11782a, main() has already registered an empty list by the time this code runs. astSpecs.isEmpty() is therefore false, and the one lookup that would have worked is skipped. The import walks zero files, writes zero records, and ant reports BUILD SUCCESSFUL:

[java] Utilizing 1 concurrent thread(s) to process 0 file(s); 16 CPU(s), 1024 MiB heap.
[java] No files found in the file set.

Every profile which declares ImportWorker is affected the same way: schema/import, schema/reindex, schema/word_reindex and schema/generate_ddl.

In short, the pool size has to be decided before the profile is loaded, but the file list can only be built correctly after it is loaded. 11782a used one call for both, at the earlier point.

The fix I recommend is to separate the two jobs, as before 11782a:

• main() only counts the files, to size the pool. It no longer registers the result.
• initialize() looks up and registers the files after the profile is loaded, exactly as before 11782a.

else if (directory != null && !callGraphMode)
{
   fileCount = <size of the file list for directory/fileSpec>;   // counted, not registered
}
...
startThreads(numThreads, explicit ? countAstSpecArtifacts(engine) : fileCount);

The change is a few lines in PatternEngine.main(). Resolving files for processing goes back to the code that worked before 11782a, and the pool for an ordinary run is sized from the same count as today.

The one visible side effect is that for the import the count is taken while jar mode is still off. The log therefore still says 0 file(s), although one file is then processed. That 0 only feeds the default pool size, which becomes 1 thread, and that is what the import uses anyway (#3211). I still plan to add -numThreads=1 explicitly to the schema/import calls in the project build files, so that this does not depend on the count.

I am no longer proposing to load the workers first, as in my previous notes. That would make the count correct (1 instead of 0), but only by changing how ConfigLoader loads every profile: workers in a first pass, the rest in a second. It also does not work for workers declared in included rule sets, so it would need its own exceptions. That is a large change to shared code just to correct a number in a log line, so I would not do it in a regression fix.

I reproduced the problem with a small test suite. Each test checks the record counts in the resulting database, not only the log:

I will rerun the same suite once the fix is in 11782b.

If you agree, I will go ahead with this in 11782b.

One more point about the fix I proposed in the previous note: the import ends up single-threaded only by accident.

The import runs on one thread today because of the 0 file(s) count. schema/import names exactly one .p2o, so the early count is either 0 or 1, and calculateOptimalThreads() turns both into one thread. That matches what we agreed in #3211, but nothing states it and nothing checks it. Passing -numThreads=4, or changing calculateOptimalThreads() (for example adding a minimum of 2), would make the import multi-threaded without any error. We have just seen how long a silent change in the import can go unnoticed.

To make it stable I would add two small things in 11782b, on top of the count-only fix:

• The callers state it: -numThreads=1 in tools/scripts/import.sh, word_reindex.sh and fts_migrate.sh, and in the import targets of the projects' build_db.xml (a separate change in each project).
• ImportWorker checks it: its constructor, which already turns jar mode on, refuses to run when the run is not single-threaded, with a message asking for -numThreads=1.

if (!PatternEngine.isSingleThreadedMode())
{
   throw new IllegalStateException("The import must run single-threaded; use -numThreads=1.");
}

The check does not choose the thread count; -numThreads still decides that. The engine does not need to know anything about schema/import either. The worker that has the requirement simply refuses a setup it cannot handle, so a wrong setup fails loudly instead of quietly. The conversion does not use ImportWorker, so a normal multi-threaded conversion cannot trip the check.

I think the premise of this is wrong. We should not embed FWD-specific concepts like import into TRPL, a facility that is supposed to be a generic language.

The import already uses multiple threads for the actual SQL, right? That is why we don't need to multi-thread at the PatternEngine level. I don't understand why we have to do gymnastics with calculating the number of files for a process that is intended to be single threaded anyway.

Isn't a cleaner approach to just know that this use case is single threaded, cleanly tell the PE that fact early enough to bypass any file counting nonsense.

We also never want to rely upon the customer to have to pass numThreads in order for this to happen.

Greg Shah wrote:

Isn't a cleaner approach to just know that this use case is single threaded, cleanly tell the PE that fact early enough to bypass any file counting nonsense.

We also never want to rely upon the customer to have to pass numThreads in order for this to happen.

I agree. Another proposal is a generic attribute on the root element of the rule set, for example <cfg singleThreaded="true">. Before it counts any files, PatternEngine.main() would read only that attribute from the profile. If it is set, main() calls setSingleThreadedMode(true) and does not count. Otherwise nothing changes, and the count and the pool sizing work as they do now for every other profile.

The engine would only know that a rule set asks for a single thread. The rule sets which need it (schema/import, schema/reindex, schema/word_reindex and schema/generate_ddl) would carry the attribute. I would then revert the check in ImportWorker and the -numThreads=1 changes in the scripts.

The cost is that main() has to locate the profile and read it once more before the normal load. I have not checked yet how cheap that is, and I will do so before implementing.

Is this the approach you want?

I'm good with that idea.

I implemented the approach from my previous note in 11782b/16811. The four rule-sets which register ImportWorker (schema/import, schema/reindex, schema/word_reindex and schema/generate_ddl) now declare <cfg singleThreaded="true">, and PatternEngine.main() skips the file counting for them and starts one thread, whatever -numThreads says. Every other profile is counted and sized as before.

The new method ConfigLoader.isSingleThreaded() reads the attribute before the profile is loaded. It returns false when the profile cannot be read, so load() still reports the error.

I removed the earlier changes of 11782b which this replaces. ImportWorker no longer refuses a multi-threaded run, and import.sh, fts_migrate.sh and word_reindex.sh are back to their trunk content.

I ran the import regression suite with Claude against the rebuilt p2j.jar, and all 35 checks passed. The import loads the same records with no option, with -numThreads=1 and with -numThreads=4, always on one thread, and the search profile still uses 16 threads over its 60 ASTs.

The log still says process 0 file(s) for these profiles, because the count is skipped. If we are sure that each of them processes exactly one file, main() can call startThreads(1, 1) for them and the message will say 1 file. Do you want me to do that?

Greg, please review the changes.

Greg Shah wrote:

Code Review Task Branch 11782b Revision 16810 and 16811

I'm OK with the changes.

Thanks for the review. I tested 11782b (revisions 16810 and 16811) on the hotel test project and everything passed:

• The import regression suite passes all 32 checks. ant import.db loads 585 records into the first test project and 76 into the second, the same as on trunk 16804, the last revision before 11782a. The direct PatternEngine schema/import run loads 585 records with no option, with -numThreads=1 and with -numThreads=4, always on one thread.
• ant import.db into PostgreSQL loads 585 records on one thread.
• word_reindex.sh and fts_migrate.sh complete against that PostgreSQL database on one thread, and the unmodified scripts no longer need -numThreads=1.
• PatternEngine.main() and SearchTrees.main() over the 60 ASTs find the same matches with the default 16 threads and with -numThreads=1 (253 and 284 hits).
• ReportApi.runSearch() in the FWD Analytics backend returns the same results with 16 threads and with 1 thread: 290 matches in 35 files for the for search and 4553 matches in 51 files for the if search, over the 60 ASTs.

I also ran the full conversion regression testing on Hotel ChUI, Hotel GUI, ChUI regression testing app, OO app and ETF, with no differences in the generated output.

The testing is complete. Please let me know if you want anything else tested, otherwise I think we can merge 11782b to trunk.

Rebased branch 11782b with trunk/16821. The new revision is 16823.

I updated the wiki for the changes of 11782a and 11782b, merged into trunk as 16805 and 16822.

• Writing TRPL Rule-Sets: the cfg element now documents the singleThreaded attribute.
• Writing Thread-Safe TRPL: the introduction says that a rule set run from the PatternEngine command line, SearchTrees and the Code Analytics search now also run concurrently. The missing multiThreadAccess message is described as shown only at debug level 3. The new section Rule Sets Which Cannot Run Concurrently explains singleThreaded, and the checklist before committing a rule set covers it.
• Running Conversion: "Thread Count" adds PatternEngine and SearchTrees to the tools which accept -numThreads, and says that the Code Analytics search sizes its pool itself and runs one search at a time. "Single-Threaded Work" lists the four schema profiles which declare singleThreaded.
• Data Migration: "Run the Import" explains that maxThreads is the only thread setting the import needs, that -numThreads has no effect, and that the startup line process 0 file(s) is expected.
• Search: "Usage" says that the report server searches on several threads and runs one search at a time.

In yesterday's IDE meeting we discussed how to cover regressions like the one fixed in 11782b with an automated test which is not based on an ABL testcase. That regression went unnoticed because ant import.db reported BUILD SUCCESSFUL although no record was imported, so the test must check the imported data, not only that the command completes.

The proposal is to build the test the same way as the preprocessor tests of the testcases project. Those are Harness tests under tests/conversion/preprocessor which run an ant target in a bash stream, wait for BUILD SUCCESSFUL and compare the output with a baseline file committed next to them. The import tests would live in a folder of their own beside them and would import the testcases dump data into a scratch H2 database, so the project's own database is never touched. They would replace the bash scripts I used to test 11782b on the hotel projects, which cannot be committed as they are.

The main check is on the records themselves. After each run, every table is exported to a CSV file sorted by primary key, without the surrogate id column, and compared with a committed baseline. The testcases dump data is small (38 .d files, 1.2 MB), so the baselines stay readable in a diff. The log is checked only for what leaves no trace in the database, such as the number of threads used.

• ant import.db.h2, as a user runs it.
• PatternEngine schema/import called directly, with no -numThreads option.
• The same call with -numThreads=1.
• The same call with -numThreads=4.

• The command ends with BUILD SUCCESSFUL, or with exit code 0 for the direct calls.
• Every table of the schema exists in the database.
• The row count of each table matches the baseline, and the total is not 0.
• The contents of each table match the baseline CSV, which also catches wrong values and rows imported twice.
• The log reports one thread (Utilizing 1 concurrent thread(s)), also when -numThreads=4 is passed.
• The log does not contain No files found in the file set, nor any error or exception.

• The four runs produce identical table exports.
• Two runs of the same command produce identical exports, which shows that the baseline holds no value which changes from one run to the next.

• schema/reindex completes on one thread after the import, with no error in the log and unchanged row counts.
• schema/word_reindex completes on one thread, and the word tables hold the rows of the baseline.

• A search profile over the testcases ASTs uses more than one thread by default.
• Its sorted matches are identical with -numThreads=1 and with -numThreads=4, and they match the baseline.

• Do we want a more generic automated test suite for the issues which cannot be tested with an ABL testcase, with the import tests as its first part, and should it be addressed in a dedicated task? Each test would run a command, reduce its result to a stable text form (a table export, selected log lines, a list of generated files with checksums) and compare it with a baseline or with a second run using other options. It could cover areas such as these:
  • The conversion of the same file set produces identical output on one thread and on several threads.
  • Schema conversion produces the expected DDL for each database dialect and the expected DMO interfaces.
  • The parser produces the expected ASTs for a set of difficult sources.
  • SearchTrees and the Code Analytics search return the expected matches.
  • The database scripts and ant targets, such as the import into PostgreSQL and MariaDB, word_reindex.sh and fts_migrate.sh, complete with the expected data.
  • gen-bundles.sh produces the expected resource bundles from the .po files.
  • The server starts with a given configuration and logs no error.
• Should schema/generate_ddl be included too? It is the fourth rule set which declares singleThreaded="true", and its check would compare the generated DDL with a baseline.