Krinkle has uploaded a new change for review. https://gerrit.wikimedia.org/r/223856
Change subject: resourceloader: Convert FileModule to use version hashing ...................................................................... resourceloader: Convert FileModule to use version hashing Enabling the module content versionining is not feasible for FileModule as that would involve Lessc and CSSJanus just to compute the version hash. Instead, we can keep the existing logic that exists for the timestamp-based versioning (which already has a comprehensive grip on tracking all involved factors that cause a module to change) and convert it to use hashing instead. This way the version hashes will be deterministic. Currently module versions tend to be invalidated too often (and sometimes not often enough) due to Git and other transport mechanisms not preserving file timestamps. == Research == * <https://phabricator.wikimedia.org/T98087#1412712> It'd take upto 10 seconds to run startup if FileModule enables build versioning. * <https://phabricator.wikimedia.org/T104950#1433142> Checking all files in resources/** took only ~30m longer in total with sha1_file compared to filemtime. Bug: T104950 Change-Id: I732fa4db32258c634e32b507952f76eac7fc9395 --- M includes/resourceloader/ResourceLoaderFileModule.php M includes/resourceloader/ResourceLoaderModule.php 2 files changed, 32 insertions(+), 24 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/56/223856/1 diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index b734def..efb151e 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -526,19 +526,16 @@ } /** - * Helper method to gather file mtimes for getDefinitionSummary. + * Helper method to gather file hashes for getDefinitionSummary. * - * Last modified timestamps are calculated from the highest last modified - * timestamp of this module's constituent files as well as the files it - * depends on. This function is context-sensitive, only performing - * calculations on files relevant to the given language, skin and debug - * mode. + * This function is context-sensitive, only computing hashes of files relevant to the + * given language, skin, etc. * * @see ResourceLoaderModule::getFileDependencies * @param ResourceLoaderContext $context * @return array */ - protected function getFileMtimes( ResourceLoaderContext $context ) { + protected function getFileHashes( ResourceLoaderContext $context ) { $files = array(); // Flatten style files into $files @@ -577,13 +574,10 @@ // entry point Less file we already know about. $files = array_values( array_unique( $files ) ); - // Don't max() because older files are significant. - // While the associated file names are significant, that is already taken care of by the - // definition summary. Avoid creating an array keyed by file path here because those are - // absolute file paths. Including that would needlessly cause global cache invalidation - // when the MediaWiki installation path changes (which is quite common in cases like - // Wikimedia where the installation path reflects the MediaWiki branch name). - return array_map( array( __CLASS__, 'safeFilemtime' ), $files ); + // Don't include keys or file paths here, only the hashes. Including that would needlessly + // cause global cache invalidation when files move or if e.g. the MediaWiki path changes. + // Any significant ordering is already detected by the definition summary. + return array_map( array( __CLASS__, 'safeFileHash' ), $files ); } /** @@ -597,6 +591,9 @@ $options = array(); foreach ( array( + // T104950: Do not include localBasePath! That path may vary over time and needlessly + // invalidate cache. If the path changes in a way that makes relative file paths point + // to something else, getFileHashes() will incorporate that already. 'scripts', 'debugScripts', 'loaderScripts', @@ -611,9 +608,6 @@ 'group', 'position', 'skipFunction', - // FIXME: localBasePath includes the MediaWiki installation path and - // needlessly causes cache invalidation. - 'localBasePath', 'remoteBasePath', 'debugRaw', 'raw', @@ -623,7 +617,7 @@ $summary[] = array( 'options' => $options, - 'fileMtimes' => $this->getFileMTimes( $context ), + 'fileHashes' => $this->getFileHashes( $context ), 'msgBlobMtime' => $this->getMsgBlobMtime( $context->getLanguage() ), ); return $summary; diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 94edb36..88829b4 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -835,16 +835,30 @@ } /** - * Safe version of filemtime(), which doesn't throw a PHP warning if the file doesn't exist - * but returns 1 instead. - * @param string $filename File name + * Safe version of filemtime(), which doesn't throw a PHP warning if the file doesn't exist. + * Defaults to 1. + * + * @param string $filePath File path * @return int UNIX timestamp */ - protected static function safeFilemtime( $filename ) { + protected static function safeFilemtime( $filePath ) { MediaWiki\suppressWarnings(); - $mtime = filemtime( $filename ) ?: 1; + $mtime = filemtime( $filePath ) ?: 1; MediaWiki\restoreWarnings(); - return $mtime; } + + /** + * Safe version of sha1_file(), which doesn't throw a PHP warning if the file doesn't exist. + * Defaults to empty string. + * + * @param string $filePath File path + * @return string Hash + */ + protected static function safeFileHash( $filePath ) { + MediaWiki\suppressWarnings(); + $hash = sha1_file( $filename ) ?: ''; + MediaWiki\restoreWarnings(); + return $hash; + } } -- To view, visit https://gerrit.wikimedia.org/r/223856 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I732fa4db32258c634e32b507952f76eac7fc9395 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Krinkle <[email protected]> _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
