Moodle

Adding David too...

The attached patch is a possible refactoring.

Warning: it is completely untested (and one of the most important tests to do is some performance tests).

I was just hoping that some people could take a look at the general idea and respond by saying either:
A) This is a stupid idea, stop it at once.
B) That looks promising, it is worth continuing to write some unit tests and performance tests, please finish it and add it to Moodle 2.0.

Just for reference - some time ago I started to write a spec that provided quite an abstract view on our language packs. I actually used it with some translation tools. See http://docs.moodle.org/en/Development:Language_API
The performance was not an issue, however, as generally translation tools provide different type os services than get_text()

What should be confirmed clearly: we stay with our own format and there is no plan to switch to some gettext (PO) or other format. Quite often people coming from other projects ask about it.

Looks a good step, from an re-organisation of code POV.

I'm not sure about all those "static" but modified properties anyway, perhaps because of my Java knowledge, but I don't feel brave enough against them, so np. I prefer 1-instance objects with "->" method calling, but it's not important.

Just guessing if we could define the non-modified properties using the "const" way, leaving static for modificable ones, basically "string_manager::$initialised". Access for consts is also provided by "::", but without $ afaik.

About PO vs get_string(), and other alternatives... I really don't know enough to opine. My point is only about memory / performance... if anyone can make some crunching about them...

Ciao

With an instance, you have to create it somewhere, and keep track of it (probably in a static member variable!) So just doing everything as static seemed easier. Anyway, I was planning to time both options and see which was faster.

Not sure about const, but it might be a good idea. I just don't think a const can be an array. Perhaps I should just move all the arrays into init instead.

And no one is proposing to change the format of our string files.

Revised patch with the typos corrected. Also a simple performance measuring script lib/simpletest/getstringperformancetester.php

The attached spreadsheet shows before and after timings for my patch, on my desktop machine.

It seems to save about 4% on average, and the only cases that got slower are the ones involving missing strings.

I think that is becuase I am avoiding repeating file_exists() calls (MDL-16181) - I think that refactoring as a class may be a tiny bit slower, even though it makes the code nicer.

It would be nice to try this script on a range of hardware/osses.

"B) That looks promising, it is worth continuing to write some unit tests and performance tests, please finish it and add it to Moodle 2.0. "

I should add, to run the performance tests, you need to install the fr and fr_ca language packs.

OK, yet another patch.

Changes in the get_string implementation:

1. Factored out locations_to_search($module) and cache the results of that in an array to save work.

2. Avoid eval altogether if strpos($result, '$') === false && strpos($result, '
') === false

3. Strip the sprintf out of the eval, and so get rid of clean_getstring_data

(1. is a small performance win.
2. is a big performance win.
3. amazingly makes not difference, but it is better without it.)

Changes to the performance script:

A. Instead of repeating the same get_string call, we instead play back the sequence of get_string calls used by various real Moodle pages. I am using admin/index.php (2000 get_strings) and course/view.php with editing on (350 get_strings).

B. To let me do that, I added some code to get_string to record the sequence of calls. I left the code in there, but commented out.

C. I added the old get_string to the end of that file, so I can do comparative timings.

Not, I find the times fluctuate a lot if you reload the test scrip, I am not sure why, but it is always worth doing several runs and averaging.

Things still to do:

• Write unit tests to ensure correctness.
• Fix help.php.

Finally, I think this is finished.

I would be very grateful if, over the weekend, people could:

1. Apply the patch (the latest get_string.patch.txt) to a HEAD install; install the fr and fr_ca language packs; and run the script .../lib/simpletest/getstringperformancetester.php.

That now compares my new code with the previous get_string implementation. You need to compare the numbers under the headings "Playing back calls from admin_index.php_old_get_string.log.php" and "Playing back calls from admin_index.php_get_string.log.php", etc. Hopefully the new numbers are faster than the old numbers on a wide range of platforms. The are on my desktop machine.

2. Review the code and/or the unit tests. Particularly, if you are familiar with tricky cases in get_string that have caused bugs in the past, please can you see if they are already covered by the unit tests, tell me what new tests we need (or write more tests yourself).

Hopefully, no-one will find any problems, and I can commit this on Monday morning.

Of course, if anyone can find any further improvements, I would love to hear about them, particularly if they are delivered in the form of an improved patch.

Tim - Since get_string is getting some attention/refactoring, I figured I would run this issue by you and see if you think it is in any way related or worth doing as part of the refactoring. If not, feel free to remove the link. Peace - Anthony

Tim, some language string in installation seems missing.

Index: lib/moodlelib.php
===================================================================
RCS file: /cvsroot/moodle/moodle/lib/moodlelib.php,v
retrieving revision 1.1184
diff -u -r1.1184 moodlelib.php
— lib/moodlelib.php 2 Apr 2009 02:42:12 -0000 1.1184
+++ lib/moodlelib.php 3 Apr 2009 07:26:29 -0000
@@ -5522,7 +5522,7 @@

if (!is_null($this->installstrings) && in_array($identifier, $this->installstrings)) { $module = 'installer'; - array_unshift($locations, $this->dirroot . '/install/lang/'); + $locations[$this->dirroot . '/install/lang/'] = ''; }

if ($extralocations) {

Anthony, if you had read the bug closely, you would see that I had already fixed the bug you link to as part of the commit.

Dongsheng. Thank you. I had not noticed that problem. Will fix.

OK done.