From 80b649127f0e660930b4a71d2a1cffd07e41c888 Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 19 Mar 2019 17:05:49 +0100 Subject: [PATCH 1/6] [WIP][PERF] FileSystem Adapter: Use native fs.copy when possible --- lib/Resource.js | 7 ++++++- lib/adapters/FileSystem.js | 34 +++++++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/lib/Resource.js b/lib/Resource.js index ecdd96f8..6682d9a2 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -40,7 +40,7 @@ class Resource { * In some cases this is the most memory-efficient way to supply resource content * @param {object} [parameters.project] Experimental, internal parameter. Do not use */ - constructor({path, statInfo, buffer, string, createStream, stream, project}) { + constructor({path, statInfo, buffer, string, createStream, stream, source, project}) { if (!path) { throw new Error("Cannot create Resource: path parameter missing"); } @@ -53,6 +53,7 @@ class Resource { this._path = path; this._name = this._getNameFromPath(path); this._project = project; // Experimental, internal parameter + this._source = source; // Experimental, internal parameter this._statInfo = statInfo || { // TODO isFile: fnTrue, isDirectory: fnFalse, @@ -323,6 +324,10 @@ class Resource { return tree; } + getSource() { + return this._source || {}; + } + /** * Returns the content as stream. * diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index ae1d1fa4..83e00cc3 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -59,6 +59,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: this._virBaseDir, + source: { + adapter: "FileSystem", + fsPath: this._fsBasePath + }, createStream: () => { return fs.createReadStream(this._fsBasePath); } @@ -91,6 +95,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: virPath, + source: { + adapter: "FileSystem", + fsPath: fsPath + }, createStream: () => { return fs.createReadStream(fsPath); } @@ -157,7 +165,10 @@ class FileSystem extends AbstractAdapter { project: this._project, statInfo: stat, path: virPath, - fsPath + source: { + adapter: "FileSystem", + fsPath + } }; if (!stat.isDirectory()) { @@ -205,6 +216,21 @@ class FileSystem extends AbstractAdapter { await makeDir(dirPath, {fs}); return new Promise((resolve, reject) => { + totalResources++; + const resourceSource = resource.getSource(); + if (resource._createStream && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { + copiedResources++; + fs.copyFile(resourceSource.fsPath, fsPath, (err) => { + if (err) { + reject(err); + } else { + resolve(); + } + }); + return; + } + + let contentStream; if (drain || readOnly) { @@ -254,4 +280,10 @@ class FileSystem extends AbstractAdapter { } } +let totalResources = 0; +let copiedResources = 0; +setInterval(() => { + console.log(`Copied ${copiedResources}/${totalResources} resources...`); +}, 1000); + module.exports = FileSystem; From c38c376813a520f2330e85666a96c09cc4750cc6 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Thu, 14 Apr 2022 12:04:11 +0200 Subject: [PATCH 2/6] [INTERNAL] Fix modified detection, skip write when src=dest --- lib/Resource.js | 13 +++++++++++++ lib/adapters/FileSystem.js | 9 ++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/lib/Resource.js b/lib/Resource.js index 6682d9a2..88a303b0 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -38,6 +38,7 @@ class Resource { * stream of the content of this resource (cannot be used in conjunction with parameters buffer, * string or stream). * In some cases this is the most memory-efficient way to supply resource content + * @param {object} [parameters.source] Experimental, internal parameter. Do not use * @param {object} [parameters.project] Experimental, internal parameter. Do not use */ constructor({path, statInfo, buffer, string, createStream, stream, source, project}) { @@ -82,6 +83,9 @@ class Resource { this.setString(string); } + // Indicator for adapters like FileSystem to detect whether a resource has been changed + this._modified = false; + // Tracing: this._collections = []; } @@ -114,6 +118,9 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { + if (!this._modified) { + this._modified = true; + } this._createStream = null; // if (this._stream) { // TODO this may cause strange issues // this._stream.destroy(); @@ -197,6 +204,9 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { + if (!this._modified) { + this._modified = true; + } this._buffer = null; // if (this._stream) { // TODO this may cause strange issues // this._stream.destroy(); @@ -366,7 +376,10 @@ class Resource { }); contentStream.on("end", () => { const buffer = Buffer.concat(buffers); + const modified = this._modified; this.setBuffer(buffer); + // Modified flag should be reset as the resource hasn't been modified from the outside + this._modified = modified; this._buffering = null; resolve(buffer); }); diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index 83e00cc3..fee5f512 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -218,7 +218,12 @@ class FileSystem extends AbstractAdapter { return new Promise((resolve, reject) => { totalResources++; const resourceSource = resource.getSource(); - if (resource._createStream && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { + if (!resource._modified && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { + if (resourceSource.fsPath === fsPath) { + // Resource is not modified and src/dest are equal => Write can be skipped + skippedResources++; + return; + } copiedResources++; fs.copyFile(resourceSource.fsPath, fsPath, (err) => { if (err) { @@ -282,8 +287,10 @@ class FileSystem extends AbstractAdapter { let totalResources = 0; let copiedResources = 0; +let skippedResources = 0; setInterval(() => { console.log(`Copied ${copiedResources}/${totalResources} resources...`); + console.log(`Skipped ${skippedResources}/${totalResources} resources...`); }, 1000); module.exports = FileSystem; From abd7dcc83b2eb5cf6bdd0441b8ed1354db9f71d5 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Tue, 19 Apr 2022 15:49:27 +0200 Subject: [PATCH 3/6] Refactoring / add logging --- lib/Resource.js | 20 +++++++++++------ lib/adapters/FileSystem.js | 44 ++++++++++++++------------------------ 2 files changed, 30 insertions(+), 34 deletions(-) diff --git a/lib/Resource.js b/lib/Resource.js index 88a303b0..c335b39a 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -55,6 +55,9 @@ class Resource { this._name = this._getNameFromPath(path); this._project = project; // Experimental, internal parameter this._source = source; // Experimental, internal parameter + if (this._source) { + this._source.modified = false; + } this._statInfo = statInfo || { // TODO isFile: fnTrue, isDirectory: fnFalse, @@ -118,8 +121,8 @@ class Resource { * @param {Buffer} buffer Buffer instance */ setBuffer(buffer) { - if (!this._modified) { - this._modified = true; + if (this._source && !this._source.modified) { + this._source.modified = true; } this._createStream = null; // if (this._stream) { // TODO this may cause strange issues @@ -204,8 +207,8 @@ class Resource { callback for dynamic creation of a readable stream */ setStream(stream) { - if (!this._modified) { - this._modified = true; + if (this._source && !this._source.modified) { + this._source.modified = true; } this._buffer = null; // if (this._stream) { // TODO this may cause strange issues @@ -376,10 +379,15 @@ class Resource { }); contentStream.on("end", () => { const buffer = Buffer.concat(buffers); - const modified = this._modified; + let modified; + if (this._source) { + modified = this._source.modified; + } this.setBuffer(buffer); // Modified flag should be reset as the resource hasn't been modified from the outside - this._modified = modified; + if (this._source) { + this._source.modified = modified; + } this._buffering = null; resolve(buffer); }); diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index fee5f512..52a31647 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -1,6 +1,8 @@ const log = require("@ui5/logger").getLogger("resources:adapters:FileSystem"); const path = require("path"); +const {promisify} = require("util"); const fs = require("graceful-fs"); +const copyFile = promisify(fs.copyFile); const globby = require("globby"); const makeDir = require("make-dir"); const {PassThrough} = require("stream"); @@ -212,30 +214,24 @@ class FileSystem extends AbstractAdapter { const fsPath = path.join(this._fsBasePath, relPath); const dirPath = path.dirname(fsPath); - log.verbose("Writing to %s", fsPath); - await makeDir(dirPath, {fs}); - return new Promise((resolve, reject) => { - totalResources++; - const resourceSource = resource.getSource(); - if (!resource._modified && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { - if (resourceSource.fsPath === fsPath) { - // Resource is not modified and src/dest are equal => Write can be skipped - skippedResources++; - return; - } - copiedResources++; - fs.copyFile(resourceSource.fsPath, fsPath, (err) => { - if (err) { - reject(err); - } else { - resolve(); - } - }); - return; + + const resourceSource = resource.getSource(); + if (!resourceSource.modified && resourceSource.adapter === "FileSystem" && resourceSource.fsPath) { + // fs.copyFile can be used when the resource is from FS and hasn't been modified + // In addition, nothing needs to be done when src === dest + if (resourceSource.fsPath === fsPath) { + log.verbose("Skip writing to %s (Resource hasn't been modified)", fsPath); + } else { + log.verbose("Copying resource from %s to %s", resourceSource.fsPath, fsPath); + await copyFile(resourceSource.fsPath, fsPath, ); } + return; + } + log.verbose("Writing to %s", fsPath); + return new Promise((resolve, reject) => { let contentStream; if (drain || readOnly) { @@ -285,12 +281,4 @@ class FileSystem extends AbstractAdapter { } } -let totalResources = 0; -let copiedResources = 0; -let skippedResources = 0; -setInterval(() => { - console.log(`Copied ${copiedResources}/${totalResources} resources...`); - console.log(`Skipped ${skippedResources}/${totalResources} resources...`); -}, 1000); - module.exports = FileSystem; From 1ec5633153414e570ba9467b735c7a8dd43bff87 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Tue, 19 Apr 2022 16:12:00 +0200 Subject: [PATCH 4/6] Fix tests --- lib/Resource.js | 4 +--- lib/adapters/FileSystem.js | 10 +++++++-- test/lib/adapters/FileSystem_write.js | 29 +++++++++++++++++++++++++++ 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/lib/Resource.js b/lib/Resource.js index c335b39a..5f2a8094 100644 --- a/lib/Resource.js +++ b/lib/Resource.js @@ -56,6 +56,7 @@ class Resource { this._project = project; // Experimental, internal parameter this._source = source; // Experimental, internal parameter if (this._source) { + // Indicator for adapters like FileSystem to detect whether a resource has been changed this._source.modified = false; } this._statInfo = statInfo || { // TODO @@ -86,9 +87,6 @@ class Resource { this.setString(string); } - // Indicator for adapters like FileSystem to detect whether a resource has been changed - this._modified = false; - // Tracing: this._collections = []; } diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index 52a31647..0586a305 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -3,12 +3,15 @@ const path = require("path"); const {promisify} = require("util"); const fs = require("graceful-fs"); const copyFile = promisify(fs.copyFile); +const chmod = promisify(fs.chmod); const globby = require("globby"); const makeDir = require("make-dir"); const {PassThrough} = require("stream"); const Resource = require("../Resource"); const AbstractAdapter = require("./AbstractAdapter"); +const READ_ONLY_MODE = 0o444; + /** * File system resource adapter * @@ -224,7 +227,10 @@ class FileSystem extends AbstractAdapter { log.verbose("Skip writing to %s (Resource hasn't been modified)", fsPath); } else { log.verbose("Copying resource from %s to %s", resourceSource.fsPath, fsPath); - await copyFile(resourceSource.fsPath, fsPath, ); + await copyFile(resourceSource.fsPath, fsPath); + if (readOnly) { + await chmod(fsPath, READ_ONLY_MODE); + } } return; } @@ -260,7 +266,7 @@ class FileSystem extends AbstractAdapter { const writeOptions = {}; if (readOnly) { - writeOptions.mode = 0o444; // read only + writeOptions.mode = READ_ONLY_MODE; } const write = fs.createWriteStream(fsPath, writeOptions); diff --git a/test/lib/adapters/FileSystem_write.js b/test/lib/adapters/FileSystem_write.js index 3c15be84..5bac557c 100644 --- a/test/lib/adapters/FileSystem_write.js +++ b/test/lib/adapters/FileSystem_write.js @@ -78,6 +78,35 @@ test("Write resource in drain mode", async (t) => { // Write resource content to another readerWriter await readerWriters.dest.write(resource, {drain: true}); + t.notThrows(() => { + assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); + }); + + // Should not fail as resource hasn't been modified + await t.notThrowsAsync(resource.getBuffer()); +}); + +test("Write modified resource in drain mode", async (t) => { + const readerWriters = t.context.readerWriters; + const destFsPath = path.join(t.context.tmpDirPath, "index.html"); + + // Get resource from one readerWriter + const resource = await readerWriters.source.byPath("/app/index.html"); + + resource.setString(` + + + Application A + + + + +` + ); + + // Write resource content to another readerWriter + await readerWriters.dest.write(resource, {drain: true}); + t.notThrows(() => { assert.fileEqual(destFsPath, "./test/fixtures/application.a/webapp/index.html"); }); From d437b7af1128bd5df3340ee59ed0e2e27bfb458e Mon Sep 17 00:00:00 2001 From: Merlin Beutlberger Date: Tue, 19 Apr 2022 16:59:18 +0200 Subject: [PATCH 5/6] FileSystem: Fix FS path comparison, write from stream to new path only * Always resolve fsBasePath to absolute path for later comparison * Always transform stream into buffer if write path equals source path --- lib/adapters/FileSystem.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/adapters/FileSystem.js b/lib/adapters/FileSystem.js index 0586a305..de61ccb0 100644 --- a/lib/adapters/FileSystem.js +++ b/lib/adapters/FileSystem.js @@ -31,7 +31,7 @@ class FileSystem extends AbstractAdapter { */ constructor({virBasePath, project, fsBasePath, excludes}) { super({virBasePath, project, excludes}); - this._fsBasePath = fsBasePath; + this._fsBasePath = path.resolve(fsBasePath); } /** @@ -240,7 +240,7 @@ class FileSystem extends AbstractAdapter { return new Promise((resolve, reject) => { let contentStream; - if (drain || readOnly) { + if ((drain || readOnly) && resourceSource.fsPath !== fsPath) { // Stream will be drained contentStream = resource.getStream(); From dbbe89fbdd02b8c4c3d233fb421dd1a7734cbc55 Mon Sep 17 00:00:00 2001 From: Matthias Osswald Date: Wed, 20 Apr 2022 15:44:02 +0200 Subject: [PATCH 6/6] Enhance tests --- test/lib/Resource.js | 88 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 78 insertions(+), 10 deletions(-) diff --git a/test/lib/Resource.js b/test/lib/Resource.js index f61c5d52..cd8e59bb 100644 --- a/test/lib/Resource.js +++ b/test/lib/Resource.js @@ -130,18 +130,36 @@ test("Resource: getStream throwing an error", (t) => { }); }); -test("Resource: setString", (t) => { - t.plan(1); - +test("Resource: setString", async (t) => { const resource = new Resource({ path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + t.is(resource.getSource().modified, false); + resource.setString("Content"); - return resource.getString().then(function(value) { - t.is(value, "Content", "String set"); + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "Content", "String set"); +}); + +test("Resource: setBuffer", async (t) => { + const resource = new Resource({ + path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + + resource.setBuffer(Buffer.from("Content")); + + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "Content", "String set"); }); test("Resource: size modification", async (t) => { @@ -204,12 +222,14 @@ test("Resource: size modification", async (t) => { t.is(await streamResource.getSize(), 23, "size for streamResource read again"); }); -test("Resource: setStream", (t) => { - t.plan(1); - +test("Resource: setStream (Stream)", async (t) => { const resource = new Resource({ path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + const stream = new Stream.Readable(); stream._read = function() {}; stream.push("I am a "); @@ -219,9 +239,34 @@ test("Resource: setStream", (t) => { resource.setStream(stream); - return resource.getString().then(function(value) { - t.is(value, "I am a readable stream!", "Stream set correctly"); + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!", "Stream set correctly"); +}); + +test("Resource: setStream (Create stream callback)", async (t) => { + const resource = new Resource({ + path: "my/path/to/resource", + source: {} // Needs to be passed in order to get the "modified" state }); + + t.is(resource.getSource().modified, false); + + resource.setStream(() => { + const stream = new Stream.Readable(); + stream._read = function() {}; + stream.push("I am a "); + stream.push("readable "); + stream.push("stream!"); + stream.push(null); + return stream; + }); + + t.is(resource.getSource().modified, true); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!", "Stream set correctly"); }); test("Resource: clone resource with buffer", (t) => { @@ -330,6 +375,29 @@ test("getBuffer from Stream content: Subsequent content requests should not thro await t.notThrowsAsync(p2); }); +test("Resource: constructor with stream", async (t) => { + const stream = new Stream.Readable(); + stream._read = function() {}; + stream.push("I am a "); + stream.push("readable "); + stream.push("stream!"); + stream.push(null); + + const resource = new Resource({ + path: "my/path/to/resource", + stream, + source: {} // Needs to be passed in order to get the "modified" state + }); + + t.is(resource.getSource().modified, false); + + const value = await resource.getString(); + t.is(value, "I am a readable stream!"); + + // modified should still be false although setBuffer is called internally + t.is(resource.getSource().modified, false); +}); + test("integration stat - resource size", async (t) => { const fsPath = path.join("test", "fixtures", "application.a", "webapp", "index.html"); const statInfo = await stat(fsPath);