Refactor compilation result #270

Cerdic · 2020-11-11T13:39:24Z

This is an implementation of #222 and fixing #268 in a efficient way

As a matter of fact, there is no BC break.
The CompilationResult is storing 3 differents lists:

parsedFiles, as it was in the Compiler Class, listing the parsed files with their timestamp
importedFiles which list all the findImport resolved as a file, with the current dir and search path. This is used for a deep check of cache validity
includedFiles which list all the @import path, whether it's resolved as a file (then it is in the importedFiles as well) or a just kept as a css @import directive. We are not using this list, but this is an output of dart-sass and while refactoring I tried to stay as close as possible.

I think that importedFiles and parsedFiles could probably be merged, but I didn't had a deep check on that and prefered to not introduce any breaking in this refactoring.
This is probably an optimisation for later.

Cerdic · 2020-11-11T13:57:02Z

As a matter of fact, there is no BC break.

this is not true because I added an argument to findImport() to provide it the current directory instead of using the property, and I'm using this argument in the Compiler, which means a break if someone had overiden this method.

We could avoid that by not using this argument in the Compiler itself (and then fallbacking to the property if no arg provided to the method) and using it only in the Cache checking.
This means without any changement everything should work as before, but if you want to use the new cache validation option, you have to also change your findImport() override.

This is a bit less clean for the findImport() method itself, but seems acceptable.

stof · 2020-11-12T09:32:18Z

src/Compiler.php

     *
     * @api
     *
+     * @deprecated


we should add the @trigger_error('...', E_USER_DEPRECATED); to report the deprecation.

stof · 2020-11-12T09:34:16Z

src/Compiler/CompilationResult.php

+    /**
+     * @var string
+     */
+    protected $css = '';


I suggest using private visibility in the new class, to avoid any BC needs on the properties.

private on all the properties, fixed

stof · 2020-11-12T09:35:21Z

src/Compiler/CompilationResult.php

+    /**
+     * @param string $css
+     */
+    public function appendCss($css)


AFAICT, this method is only used to manage the prefix, where you even set the prefix as CSS and then append the actual CSS with this method. I'd rather not have this method here, and keep the concatenation in the compiler class.

stof · 2020-11-12T09:36:01Z

src/Compiler/CompilationResult.php

+     */
+    public function addParsedFile($path)
+    {
+        if (isset($path) && is_file($path)) {


this isset is useless useless if the param is really a string and not string|null

In fact it's string|null and I prefer to keep this as is as I don't want to introduce a test here
https://github.com/scssphp/scssphp/blob/master/src/Compiler.php#L481
which would tend to make someone move the previous line in the if whereas we need to fill the array whatever the value of $path

please change it to a null check then to be clearer

stof · 2020-11-12T09:36:44Z

src/Compiler/CompilationResult.php

+
+    /**
+     * Store the sourceMap and its storage data
+     * @param $sourceMap


missing type

stof · 2020-11-12T09:37:05Z

src/Compiler/CompilationResult.php

+    /**
+     * The sourceMap content, if it was generated
+     *
+     * @return string


should be string|null

stof · 2020-11-12T09:37:54Z

src/Compiler/CompilationResult.php

+ */
+class CompilationResult
+{
+    protected static $resolvedImport = [];


why static ?

Static is used here for performance issue accros multiples file compilation sharing mixins or functions libs.

My testing use case is 4 .scss compilation: the full BS4 as a first and then 3 others using parts of BS4 (like variables, mixins..). Without the static a lot of validation are occuring multiple times and this is costly.

Using this static allows a total cost for this full check that is quite acceptable.

I guess to be bulletproof, the checkValid() method should have an optional $forceRefresh argument to empty the static and force a full re-check?

if we need to share some state between compilations, I think this should live in the Compiler class rather than being in a static property in CompilationResult. Each compilation has its own CompilationResult.

btw, using a static property here would even leak the shared state between multiple Compiler instances, which might have different configuration.

This is purely an internal storage stuff and the case of multiple compiler instances with different configurations is taken in account through the hash, to reuse previous check only if
f57610d#diff-a95b369f4118d2d8d85bb94f822b8d5ca7e92c93a9c0d533e43742b8f3e6e742R277

In my test case indeed each scss compilation occurs with a new Compiler Instance - but I guess I could now use a static instance to reuse it accross several compilations.

Alternatively I can remove the static optimization from the CompilerResult class, delivering something ready to use and 100% clean - even if not the fastest implementation.
And besides that add an option to the getImportedFiles() method to return the full array and not just one column.

Then it allows users to implement their own checking method, possibly faster with some compromise on compilation isolation.

For me this is making a difference:

the fast checking method, with only the timestamps is around ~10ms for the whole scss compilations

with a deep checking with non-static array optimisation : ~85ms

with a deep checking with static array optimisation : ~45ms

But I also have to admit the difference is not as big as I had with my first implementation where i measured a total of ~250ms with a full check (but I can't figure out what I changed since).

Let's go for this compromise?

so, no more static, just a local variable

- store the produced CSS, SourceMap and parsedFiles - export the sourceMap to a file if needed, and produce the soucemap css to be added to the end of the css output - __toString() method return the full css including sourceMap if any, for backward compatibility - has a checkValid() method to check if the produced css is still valid (no file modified, this has to be improved) - this is the result returned by the compile() method - this is the cached object if any cache used See #222

…he sourceMap is now saved in a file by the CompilationResult if needed

… class + the list stores not only the resolved filePath but also the path and the currentDirectory used to resolve it (to be able to check later that findImport() would produce the same resolution) + findImport() method takes the currentDir as an argument to be able to call it out of the compiler to check later the importedFiles list

…ePath, as dart-sass is doing, even if we don't need that for now

…full check of unchanged findImport() before accepting a cache as valid The import resolution checking is stored in a class static property to avoid several identical checks which are involving a lot of disk access. This has a noticeable impact when compiling several files in a row, and using a framework and each file is using part of this framework. The bigger the framework the bigger the benefit :) The compilation result also has an additional flag isCached set to true when the result is coming from the cache, and checkValid() will return true without any check if the result is not coming from the cache. This allow the caller to call the checkValid() method without performance impact when the result has just been compiled.

…sult isolated from each other

stof · 2021-01-20T10:23:11Z

src/Compiler.php

-            $this->appendRootDirective('@import ' . $this->compileImportPath($rawPath) . ';', $out);
+            $path = $this->compileImportPath($rawPath);
+            $this->appendRootDirective('@import ' . $path . ';', $out);
+            $this->compilationResult->addIncludedFile($path);


this looks wrong to me. This file is not included as it is compiled to a CSS @import (and might not even be a file that exists on disk)

I will dig again, but as long I can remember I sticked to what dart-saas was doing on this files liste, even if it doesn't feels what you are expecting.

dart-sass is only adding values in this file list for sass imports, not for CSS imports (which are compiled by visitStaticImport)

the doc about filesystem import vs URI is related to the fact that dart-sass supports using URIs for imports, not only paths (when running the dart API, it can support package: URIs which resolve to a dart package installed in the project for instance)

stof · 2021-01-20T10:23:28Z

src/Compiler.php

-                    $this->appendRootDirective('@import ' . $this->compileImportPath($rawPath) . ';', $out);
+                    $path = $this->compileImportPath($rawPath);
+                    $this->appendRootDirective('@import ' . $path . ';', $out);
+                    $this->compilationResult->addIncludedFile($path);


this is not an included file either

stof · 2021-01-20T10:23:44Z

src/Compiler.php

-        $this->appendRootDirective('@import ' . $this->compileImportPath($rawPath) . ';', $out);
+        $path = $this->compileImportPath($rawPath);
+        $this->appendRootDirective('@import ' . $path . ';', $out);
+        $this->compilationResult->addIncludedFile($path);


this is not an included file either.

stof · 2021-01-20T10:24:15Z

src/Compiler.php

    public function getParsedFiles()
    {
-        return $this->parsedFiles;
+        @trigger_error('addParsedFile() is no longer a method from Compiler but from CompilationResult.'


wrong method name

stof · 2021-01-20T10:25:15Z

src/Compiler/CompilationResult.php

+    //
+    // This can be passed to [sourceMap]'s [Mapping.spanFor] method. It's `null`
+    // if source mapping was disabled for this compilation.
+    public function getSourceFiles()


why do we have such method ?

stof · 2021-01-20T10:26:32Z

I'm actually wondering whether this caching layer doing complex IO-based invalidation (as it has to resolve all imports again) makes sense or whether we should rather drop it. dart-sass does not have any such feature.

Cerdic requested a review from stof November 11, 2020 13:39

stof reviewed Nov 12, 2020

View reviewed changes

Cerdic force-pushed the refactor-compilation-result branch from f57610d to 7f6828a Compare January 7, 2021 17:00

Cerdic added 11 commits January 20, 2021 10:48

the saveMap() method from SourceMapGenerator class is deprecated as t…

668d013

…he sourceMap is now saved in a file by the CompilationResult if needed

Save the full @import list, including the urls not resolving to a fil…

584e5dc

…ePath, as dart-sass is doing, even if we don't need that for now

trigger_error() on deprecated methods

40223d3

private visibility in the class, to avoid any need to manage BC

1c5902d

missing types in the PHPDoc

a83392b

No static var for the $resolvedImport var to keep any CompilationRe…

87a27a0

…sult isolated from each other

No appendCss() method, not required and keep it as simple as possible

c7f7be4

!is_null() instead of isset() makes the code clearer

9d83d89

Cerdic force-pushed the refactor-compilation-result branch from a757feb to 9d83d89 Compare January 20, 2021 09:52

Cerdic merged commit 92882f1 into master Jan 20, 2021

Cerdic deleted the refactor-compilation-result branch January 20, 2021 10:12

This was referenced Jan 20, 2021

[RFC] Stateless API for the compiler #222

Closed

Improve the compilation cache invalidation options #268

Closed

stof reviewed Jan 20, 2021

View reviewed changes

stof added this to the 1.5 milestone Jan 21, 2021

stof mentioned this pull request Mar 11, 2021

Rework CompilationResult #315

Merged

Cerdic mentioned this pull request Mar 11, 2021

Should ScssPhp provide the Cache mecanism? #316

Closed

stof mentioned this pull request Mar 12, 2021

[RFC] Refine the compilation API #320

Closed

Refactor compilation result #270

Refactor compilation result #270

Uh oh!

Conversation

Cerdic commented Nov 11, 2020 • edited by stof Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cerdic commented Nov 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jan 20, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cerdic commented Nov 11, 2020 •

edited by stof

Loading