Jackmcbarn has uploaded a new change for review.
https://gerrit.wikimedia.org/r/178698
Change subject: Avoid unnecessary database queries
......................................................................
Avoid unnecessary database queries
Currently, mw.title.new always results in a database query, which holds up
the parse until it finishes. This changes it to not require a database
query if it's not actually necessary.
Bug: T68328
Change-Id: I62f347d4cd9176bd0440215dcbe804c1dc3d4c99
---
M engines/LuaCommon/TitleLibrary.php
M engines/LuaCommon/lualib/mw.title.lua
M tests/engines/LuaCommon/TitleLibraryTest.php
M tests/engines/LuaCommon/TitleLibraryTests.lua
4 files changed, 101 insertions(+), 51 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Scribunto
refs/changes/98/178698/1
diff --git a/engines/LuaCommon/TitleLibrary.php
b/engines/LuaCommon/TitleLibrary.php
index c3db982..9f3c5f1 100644
--- a/engines/LuaCommon/TitleLibrary.php
+++ b/engines/LuaCommon/TitleLibrary.php
@@ -12,6 +12,7 @@
$lib = array(
'newTitle' => array( $this, 'newTitle' ),
'makeTitle' => array( $this, 'makeTitle' ),
+ 'getExpensiveData' => array( $this, 'getExpensiveData'
),
'getUrl' => array( $this, 'getUrl' ),
'getContent' => array( $this, 'getContent' ),
'getFileInfo' => array( $this, 'getFileInfo' ),
@@ -19,7 +20,7 @@
'cascadingProtection' => array( $this,
'cascadingProtection' ),
);
return $this->getEngine()->registerInterface( 'mw.title.lua',
$lib, array(
- 'thisTitle' => $this->returnTitleToLua(
$this->getTitle() ),
+ 'thisTitle' => $this->getInexpensiveTitleData(
$this->getTitle() ),
'NS_MEDIA' => NS_MEDIA,
) );
}
@@ -50,58 +51,92 @@
}
/**
- * Extract information from a Title object for return to Lua
- *
- * This also records a link to this title in the current ParserOutput
- * and caches the title for repeated lookups. The caller should call
- * incrementExpensiveFunctionCount() if necessary.
+ * Extract inexpensive information from a Title object for return to Lua
*
* @param $title Title Title to return
* @return array Lua data
*/
- private function returnTitleToLua( Title $title ) {
- // Cache it
- $this->titleCache[$title->getPrefixedDBkey()] = $title;
- if ( $title->getArticleID() > 0 ) {
- $this->idCache[$title->getArticleID()] = $title;
- }
-
- // Record a link
- if ( $this->getParser() && !$title->equals( $this->getTitle() )
) {
- $this->getParser()->getOutput()->addLink( $title );
- }
-
+ private function getInexpensiveTitleData( Title $title ) {
$ns = $title->getNamespace();
$ret = array(
'isLocal' => (bool)$title->isLocal(),
- 'isRedirect' => (bool)$title->isRedirect(),
'interwiki' => $title->getInterwiki(),
'namespace' => $ns,
'nsText' => $title->getNsText(),
'text' => $title->getText(),
- 'id' => $title->getArticleID(),
'fragment' => $title->getFragment(),
- 'contentModel' => $title->getContentModel(),
'thePartialUrl' => $title->getPartialURL(),
);
- if ( $ns === NS_SPECIAL ) {
+ switch( $ns ) {
+ case NS_SPECIAL:
+ $ret['exists'] =
(bool)SpecialPageFactory::exists( $title->getDBkey() );
+ // fall through
+ default:
+ $ret['file'] = false;
+ break;
+ case NS_FILE:
+ case NS_MEDIA:
+ // do nothing
+ }
+ return $ret;
+ }
+
+ /**
+ * Extract expensive information from a Title object for return to Lua
+ *
+ * This records a link to this title in the current ParserOutput and
caches the
+ * title for repeated lookups. It may call
incrementExpensiveFunctionCount() if
+ * the title is not already cached.
+ *
+ * @param string $text Title text
+ * @return array Lua data
+ */
+ public function getExpensiveData( $text ) {
+ $this->checkType( 'getExpensiveData', 1, $text, 'string' );
+ $title = Title::newFromText( $text );
+ if ( !$title ) {
+ return array( null );
+ }
+ $dbKey = $title->getPrefixedDBkey();
+ if ( isset( $this->titleCache[$dbKey] ) ) {
+ // It was already cached, so we already did the
expensive work and added a link
+ $title = $this->titleCache[$dbKey];
+ } else {
+ if ( !$title->equals( $this->getTitle() ) ) {
+ $this->incrementExpensiveFunctionCount();
+
+ // Record a link
+ if ( $this->getParser() ) {
+
$this->getParser()->getOutput()->addLink( $title );
+ }
+ }
+
+ // Cache it
+ $this->titleCache[$dbKey] = $title;
+ if ( $title->getArticleID() > 0 ) {
+ $this->idCache[$title->getArticleID()] = $title;
+ }
+ }
+
+ $ret = array(
+ 'isRedirect' => (bool)$title->isRedirect(),
+ 'id' => $title->getArticleID(),
+ 'contentModel' => $title->getContentModel(),
+ );
+ if ( $title->getNamespace() === NS_SPECIAL ) {
$ret['exists'] = (bool)SpecialPageFactory::exists(
$title->getDBkey() );
} else {
// bug 70495: don't just check whether the ID != 0
$ret['exists'] = $title->exists();
}
- if ( $ns !== NS_FILE && $ns !== NS_MEDIA ) {
- $ret['file'] = false;
- }
- return $ret;
+ return array( $ret );
}
/**
* Handler for title.new
*
* Calls Title::newFromID or Title::newFromTitle as appropriate for the
- * arguments, and may call incrementExpensiveFunctionCount() if the
title
- * is not already cached.
+ * arguments.
*
* @param $text_or_id string|int Title or page_id to fetch
* @param $defaultNamespace string|int Namespace name or number to use
if $text_or_id doesn't override
@@ -117,7 +152,9 @@
$title = Title::newFromID( $text_or_id );
$this->idCache[$text_or_id] = $title;
}
- if ( !$title ) {
+ if ( $title ) {
+ $this->titleCache[$title->getPrefixedDBkey()] =
$title;
+ } else {
return array( null );
}
} elseif ( $type === 'string' ) {
@@ -129,25 +166,18 @@
if ( !$title ) {
return array( null );
}
- if ( isset(
$this->titleCache[$title->getPrefixedDBkey()] ) ) {
- // Use the cached version, because that has
already been loaded from the database
- $title =
$this->titleCache[$title->getPrefixedDBkey()];
- } else {
- $this->incrementExpensiveFunctionCount();
- }
} else {
// This will always fail
$this->checkType( 'title.new', 1, $text_or_id, 'number
or string' );
}
- return array( $this->returnTitleToLua( $title ) );
+ return array( $this->getInexpensiveTitleData( $title ) );
}
/**
* Handler for title.makeTitle
*
- * Calls Title::makeTitleSafe, and may call
- * incrementExpensiveFunctionCount() if the title is not already cached.
+ * Calls Title::makeTitleSafe.
*
* @param $ns string|int Namespace
* @param $text string Title text
@@ -167,14 +197,8 @@
if ( !$title ) {
return array( null );
}
- if ( isset( $this->titleCache[$title->getPrefixedDBkey()] ) ) {
- // Use the cached version, because that has already
been loaded from the database
- $title = $this->titleCache[$title->getPrefixedDBkey()];
- } else {
- $this->incrementExpensiveFunctionCount();
- }
- return array( $this->returnTitleToLua( $title ) );
+ return array( $this->getInexpensiveTitleData( $title ) );
}
// May call the following Title methods:
diff --git a/engines/LuaCommon/lualib/mw.title.lua
b/engines/LuaCommon/lualib/mw.title.lua
index 8594e4a..feb6947 100644
--- a/engines/LuaCommon/lualib/mw.title.lua
+++ b/engines/LuaCommon/lualib/mw.title.lua
@@ -195,6 +195,12 @@
k = 'fileExists'
end
+ if ( k == 'exists' or k == 'isRedirect' or k ==
'contentModel' or k == 'id' ) and not data[k] then
+ for k,v in pairs( php.getExpensiveData(
data.prefixedText ) ) do
+ data[k] = v
+ end
+ end
+
if k == 'fullText' then
if data.fragment ~= '' then
return data.prefixedText .. '#' ..
data.fragment
diff --git a/tests/engines/LuaCommon/TitleLibraryTest.php
b/tests/engines/LuaCommon/TitleLibraryTest.php
index 281cc28..dc78666 100644
--- a/tests/engines/LuaCommon/TitleLibraryTest.php
+++ b/tests/engines/LuaCommon/TitleLibraryTest.php
@@ -124,7 +124,7 @@
$this->assertFalse( isset(
$links[NS_PROJECT]['Referenced_from_Lua'] ) );
$interpreter->callFunction(
- $interpreter->loadString( 'mw.title.new(
"Project:Referenced from Lua" )', 'reference title' )
+ $interpreter->loadString( 'local _ = mw.title.new(
"Project:Referenced from Lua" ).id', 'reference title' )
);
$links = $engine->getParser()->getOutput()->getLinks();
diff --git a/tests/engines/LuaCommon/TitleLibraryTests.lua
b/tests/engines/LuaCommon/TitleLibraryTests.lua
index 7619b9f..9126c01 100644
--- a/tests/engines/LuaCommon/TitleLibraryTests.lua
+++ b/tests/engines/LuaCommon/TitleLibraryTests.lua
@@ -42,16 +42,30 @@
return tostring( title ), tostring( title.fragment )
end
-local function test_expensive()
+local function test_expensive_10()
for i = 1, 10 do
- mw.title.new( tostring( i ) )
+ local _ = mw.title.new( tostring( i ) ).id
+ end
+ return 'did not error'
+end
+
+local function test_expensive_11()
+ for i = 1, 11 do
+ local _ = mw.title.new( tostring( i ) ).id
end
return 'did not error'
end
local function test_expensive_cached()
for i = 1, 100 do
- mw.title.new( 'Title' )
+ local _ = mw.title.new( 'Title' ).id
+ end
+ return 'did not error'
+end
+
+local function test_expensive_inexpensive()
+ for i = 1, 100 do
+ local _ = mw.title.new( 'Title' ).prefixedText
end
return 'did not error'
end
@@ -356,10 +370,16 @@
expect = {
'{{int:mainpage}}<includeonly>...</includeonly><noinclude>...</noinclude>' }
},
- { name = 'expensive functions', func = test_expensive,
+ { name = 'not quite too many expensive functions', func =
test_expensive_10,
+ expect = { 'did not error' }
+ },
+ { name = 'too many expensive functions', func = test_expensive_11,
expect = 'too many expensive function calls'
},
- { name = 'expensive cached', func = test_expensive_cached,
+ { name = "previously cached titles shouldn't count as expensive", func
= test_expensive_cached,
+ expect = { 'did not error' }
+ },
+ { name = "inexpensive actions shouldn't count as expensive", func =
test_expensive_inexpensive,
expect = { 'did not error' }
},
}
--
To view, visit https://gerrit.wikimedia.org/r/178698
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62f347d4cd9176bd0440215dcbe804c1dc3d4c99
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Scribunto
Gerrit-Branch: master
Gerrit-Owner: Jackmcbarn <jackmcb...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
