Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(#2764): caching logic changed, added StHash, StHashTest, fix tests #2771

Closed
wants to merge 2 commits into from
Closed

fix(#2764): caching logic changed, added StHash, StHashTest, fix tests #2771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
974e87f
fix(#2764): caching logic changed, added StHash, StHashTest, fix tests
Yanich96 Jan 12, 2024
71ed700
fix(#2764): deleted reflection, created class Hash
Yanich96 Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 4 additions & 18 deletions eo-maven-plugin/src/main/java/org/eolang/maven/optimization/OptCached.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.BasicFileAttributes;
import java.time.ZonedDateTime;
import java.time.temporal.ChronoUnit;
import java.util.Optional;
import org.eolang.maven.AssembleMojo;
import org.eolang.maven.Place;
Expand All @@ -41,12 +38,6 @@
* Returns already optimized XML if it's found in the cache.
*
* @since 0.28.11
* @todo #2746:30min Use checksum, not time.
* The following tests show that fetching from the cache doesn't work correctly:
* - {@link OptCachedTest#returnsFromCacheCorrectProgram(Path path)},
* - {@link OptCachedTest#returnsFromCacheButTimesSaveAndExecuteDifferent(Path path)}.
* Need to fix the file validation from cache: using checksum, but not time.
* Don't forget to enable the tests.
*/
public final class OptCached implements Optimization {

Expand Down Expand Up @@ -113,16 +104,11 @@ private Path cached(final XML xml) {
*/
private boolean contains(final XML xml) throws IOException {
final Path path = this.cached(xml);
final Optional<String> time = xml.xpath("/program/@time").stream().findFirst();
final Optional<String> hash = xml.xpath("/program/@hash").stream().findFirst();
final boolean res;
if (Files.exists(path) && time.isPresent()) {
res = Files.readAttributes(path, BasicFileAttributes.class)
.creationTime()
.toInstant()
.truncatedTo(ChronoUnit.MINUTES)
.equals(
ZonedDateTime.parse(time.get()).toInstant().truncatedTo(ChronoUnit.MINUTES)
);
if (Files.exists(path) && hash.isPresent()) {
res = new XMLDocument(path).xpath("/program/@hash").stream().findFirst()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanich96 We use OptCached in order to avoid repeated unnecessary computations and increase the compilation speed. However the solution you provided might make the optimization even longer. Let's me explain.

In order to read /program/@hash from the path you need to read the entire file. This operation is extremely long. I even bet that if you disable caching now, the compilation will be faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@volodya-lombrozo
Yes, but I have 3 points:

  1. File won't be read from disk each time, thanks page cache https://habr.com/ru/companies/smart_soft/articles/228937/
  2. In most languages reading from cache is faster, than compilation. Isn't it true for EO lang?
  3. If reading from cache takes longer, then compilation.. so why the cache exists?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanich96, I conducted a simple benchmark to compare the time spent in OptCache.apply for the returnsFromCacheIfXmlAlreadyInCache test in the master branch (the old solution) and your solution. The results I obtained are as follows:

Test Scenario Master Branch (ms) Your Branch (ms)
returnsFromCacheIfXmlAlreadyInCache Test 3654, 3680, 3693, 3700, 3862 4281, 4602, 4378, 4364, 4504

As you can see, the "page cache" doesn't provide much improvement. Please note that in the test, we are using an extremely small program example (only a few lines). Real programs will be much larger, and the performance gap will likely be more significant.

Below is the test code, where I modified the test to include time measurement.

Master branch:

@Test
void returnsFromCacheIfXmlAlreadyInCache(@TempDir final Path tmp) throws IOException {
    final XML program = OptCachedTest.program(ZonedDateTime.now());
    OptCachedTest.save(tmp, program);
    final long start = System.currentTimeMillis();
    XML apply = null;
    for (int i = 0; i < 10_000; ++i) {
        apply = new OptCached(
            path -> {
                throw new IllegalStateException("This code shouldn't be executed");
            },
            tmp
        ).apply(program);
    }
    final long finish = System.currentTimeMillis();
    Logger.info(
        this,
        "Optimization took %d ms",
        finish - start
    );
    MatcherAssert.assertThat(
        "We expected that the program will be returned from the cache.",
        apply,
        Matchers.equalTo(program)
    );
}

Your branch:

@Test
void returnsFromCacheIfXmlAlreadyInCache(@TempDir final Path tmp)
    throws IOException, ImpossibleModificationException, NoSuchAlgorithmException {
    final XML program = OptCachedTest.updatedProgram(
        OptCachedTest.program(ZonedDateTime.now())
    );
    OptCachedTest.save(tmp, program);
    final long start = System.currentTimeMillis();
    XML apply = null;
    for (int i = 0; i < 10_000; ++i) {
        apply = new OptCached(
            path -> {
                throw new IllegalStateException("This code shouldn't be executed");
            },
            tmp
        ).apply(program);
    }
    final long finish = System.currentTimeMillis();
    Logger.info(
        this,
        "Optimization took %d ms",
        finish - start
    );
    MatcherAssert.assertThat(
        "We expected that the program will be returned from the cache.",
        apply,
        Matchers.equalTo(program)
    );
}

.equals(hash);
} else {
res = false;
}
Expand Down
25 changes: 16 additions & 9 deletions eo-maven-plugin/src/test/java/org/eolang/maven/OptimizeMojoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,17 +33,18 @@
import java.util.stream.IntStream;
import javax.xml.transform.TransformerFactory;
import net.sf.saxon.TransformerFactoryImpl;
import org.cactoos.io.ResourceOf;
import org.cactoos.Text;
import org.cactoos.io.InputOf;
import org.cactoos.text.TextOf;
import org.eolang.jucs.ClasspathSource;
import org.eolang.maven.util.HmBase;
import org.eolang.parser.CheckPack;
import org.eolang.parser.EoSyntax;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.hamcrest.io.FileMatchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
Expand Down Expand Up @@ -113,16 +114,22 @@ void optimizesIfExpired(@TempDir final Path temp) throws Exception {
*
* @param temp Temporary test directory.
* @throws Exception if unexpected error happened.
* @todo #2422:60min This test is unstable for now.
* We should resolve issues with unstable failures and only
* then enable the test.
* Also, see this <a href="https://github.com/objectionary/eo/issues/2727">issue</a>.
*/
@Disabled
@Test
void getsAlreadyOptimizedResultsFromCache(@TempDir final Path temp) throws Exception {
final TextOf cached = new TextOf(
new ResourceOf("org/eolang/maven/optimize/main.xml")
final Text cached = new TextOf(
new EoSyntax(
"test-it-4",
new InputOf(
String.join(
"\n",
"+alias stdout org.eolang.io.stdout",
"+package f\n",
"[x] > main",
" (stdout \"Hello!\" x).print > @"
)
)
).parsed().toString()
);
final Path cache = temp.resolve("cache");
final String hash = "abcdef1";
Expand Down
21 changes: 16 additions & 5 deletions eo-maven-plugin/src/test/java/org/eolang/maven/ShakeMojoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,16 @@
import java.nio.file.Paths;
import java.util.Map;
import java.util.concurrent.TimeUnit;
import org.cactoos.io.ResourceOf;
import org.cactoos.Text;
import org.cactoos.io.InputOf;
import org.cactoos.text.TextOf;
import org.eolang.maven.util.HmBase;
import org.eolang.parser.EoSyntax;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.hamcrest.io.FileMatchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

Expand Down Expand Up @@ -82,11 +83,21 @@ void shakesSuccessfully(@TempDir final Path temp) throws IOException {
);
}

@Disabled
@Test
void getsAlreadyShakenResultsFromCache(@TempDir final Path temp) throws Exception {
final TextOf cached = new TextOf(
new ResourceOf("org/eolang/maven/optimize/main.xml")
final Text cached = new TextOf(
new EoSyntax(
"test-it-4",
new InputOf(
String.join(
"\n",
"+alias stdout org.eolang.io.stdout",
"+package f\n",
"[x] > main",
" (stdout \"Hello!\" x).print > @"
)
)
).parsed().toString()
);
final Path cache = temp.resolve("cache");
final String hash = "abcdef1";
Expand Down
51 changes: 37 additions & 14 deletions eo-maven-plugin/src/test/java/org/eolang/maven/optimization/OptCachedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,38 +29,40 @@
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.NoSuchAlgorithmException;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import org.eolang.maven.util.HmBase;
import org.eolang.parser.StHash;
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.hamcrest.io.FileMatchers;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import org.xembly.Directives;
import org.xembly.ImpossibleModificationException;
import org.xembly.Xembler;

/**
* Test case for {@link org.eolang.maven.optimization.OptCached}.
* @since 0.28.12
*/
@SuppressWarnings("PMD.TooManyMethods")
final class OptCachedTest {

/**
* Test case for XML program in cache.
*
* @param tmp Temp dir
* @throws IOException if I/O fails
* @todo #2422:60min returnsFromCacheIfXmlAlreadyInCache: this test is unstable.
* We should resolve issues with unstable failures and only
* then enable the test.
* Also, see this <a href="https://github.com/objectionary/eo/issues/2727">issue</a>.
*/
@Disabled
@Test
void returnsFromCacheIfXmlAlreadyInCache(@TempDir final Path tmp) throws IOException {
final XML program = OptCachedTest.program(ZonedDateTime.now());
void returnsFromCacheIfXmlAlreadyInCache(@TempDir final Path tmp)
throws IOException, ImpossibleModificationException, NoSuchAlgorithmException {
final XML program = OptCachedTest.updatedProgram(
OptCachedTest.program(ZonedDateTime.now())
);
OptCachedTest.save(tmp, program);
MatcherAssert.assertThat(
"We expected that the program will be returned from the cache.",
Expand All @@ -74,11 +76,12 @@ void returnsFromCacheIfXmlAlreadyInCache(@TempDir final Path tmp) throws IOExcep
);
}

@Disabled
@Test
void returnsFromCacheButTimesSaveAndExecuteDifferent(@TempDir final Path tmp)
throws IOException {
final XML program = OptCachedTest.program(ZonedDateTime.now().minusMinutes(2));
throws IOException, ImpossibleModificationException, NoSuchAlgorithmException {
final XML program = OptCachedTest.updatedProgram(
OptCachedTest.program(ZonedDateTime.now())
);
OptCachedTest.save(tmp, program);
MatcherAssert.assertThat(
"We expected that the not immediately saved program will be returned from the cache.",
Expand All @@ -92,13 +95,16 @@ void returnsFromCacheButTimesSaveAndExecuteDifferent(@TempDir final Path tmp)
);
}

@Disabled
@Test
void returnsFromCacheCorrectProgram(@TempDir final Path tmp)
throws IOException {
final XML prev = OptCachedTest.program(ZonedDateTime.now(), "first program");
throws IOException, ImpossibleModificationException, NoSuchAlgorithmException {
final XML prev = OptCachedTest.updatedProgram(
OptCachedTest.program(ZonedDateTime.now(), "first program")
);
OptCachedTest.save(tmp, prev);
final XML current = OptCachedTest.program(ZonedDateTime.now(), "second program");
final XML current = OptCachedTest.updatedProgram(
OptCachedTest.program(ZonedDateTime.now(), "second program")
);
MatcherAssert.assertThat(
"Expecting current program to be compiled, but prev program was returned from cache.",
new OptCached(
Expand Down Expand Up @@ -199,9 +205,26 @@ private static XML program(final ZonedDateTime time, final String something) {
.add("program")
.attr("name", "main")
.attr("time", time.format(DateTimeFormatter.ISO_INSTANT))
.add("objects")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanich96 Why do you need this row?

.attr("something", something)
.up()
).xmlQuietly()
);
}

/**
* Adds attribute "hash" in EO program for tests.
* @param xml XML.
* @return XML representation of program.
*/
private static XML updatedProgram(final XML xml)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanich96 Maybe it's better to leave just program(final XML xml)?
https://www.yegor256.com/2015/01/12/compound-name-is-code-smell.html

throws NoSuchAlgorithmException, ImpossibleModificationException {
final String hash = new StHash.Hash(xml).getHash();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanich96 Is it possible to inline this variable?
https://www.yegor256.com/2015/09/01/redundant-variables-are-evil.html

return new XMLDocument(
new Xembler(
new Directives()
.xpath("//program").attr("hash", hash)
).apply(xml.node())
);
}
}
13 changes: 8 additions & 5 deletions eo-parser/src/main/java/org/eolang/parser/EoSyntax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.jcabi.log.Logger;
import com.jcabi.xml.XML;
import com.jcabi.xml.XMLDocument;
import com.yegor256.xsline.Xsline;
import java.io.IOException;
import java.util.List;
import org.antlr.v4.runtime.CommonTokenStream;
Expand Down Expand Up @@ -70,7 +71,7 @@ public EoSyntax(final String nme, final Input ipt) {
}

/**
* Compile it to XML and save.
* Compile it to XML.
*
* <p>No exception will be thrown if the syntax is invalid. In any case, XMIR will
* be generated and saved. Read it in order to find the errors,
Expand All @@ -92,10 +93,12 @@ public XML parsed() throws IOException {
parser.addErrorListener(spy);
final XeEoListener xel = new XeEoListener(this.name);
new ParseTreeWalker().walk(xel, parser.program());
final XML dom = new XMLDocument(
new Xembler(
new Directives(xel).append(spy)
).domQuietly()
final XML dom = new Xsline(new StHash()).pass(
new XMLDocument(
new Xembler(
new Directives(xel).append(spy)
).domQuietly()
)
);
new Schema(dom).check();
if (spy.size() == 0) {
Expand Down
11 changes: 7 additions & 4 deletions eo-parser/src/main/java/org/eolang/parser/PhiSyntax.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.jcabi.log.Logger;
import com.jcabi.xml.XML;
import com.jcabi.xml.XMLDocument;
import com.yegor256.xsline.Xsline;
import java.io.IOException;
import org.antlr.v4.runtime.CharStreams;
import org.antlr.v4.runtime.CommonTokenStream;
Expand Down Expand Up @@ -84,10 +85,12 @@ public XML parsed() throws IOException {
parser.removeErrorListeners();
parser.addErrorListener(spy);
new ParseTreeWalker().walk(xel, parser.program());
final XML dom = new XMLDocument(
new Xembler(
new Directives(xel).append(spy)
).domQuietly()
final XML dom = new Xsline(new StHash()).pass(
new XMLDocument(
new Xembler(
new Directives(xel).append(spy)
).domQuietly()
)
);
new Schema(dom).check();
if (spy.size() == 0) {
Expand Down
Loading
Loading