Question - Refactoring the indexing.

[ghost]

Hello

I've been taking a look at the current indexing and interaction between CoffeeScript and the PHP indexing and was thinking about possible improvements. I noticed that ClassMapRefresh maintains a JSON index file and CoffeeScript reads this when the classes are requested. This form of caching seems good to me, as it cuts down the amount of times PHP needs to be invoked to ask for information. Sadly, it is only used when fetching a list of classes and not for e.g. members of classes, which could improve responsiveness in several area's as now each call to the proxy does a PHP call.

I was wondering if if might be interesting to take the 'caching' one step further and use the index JSON file for everything? This raises the following questions:

• When we save a file, the PHP side currently recreates the file and the CoffeeScript side reads it again. Is loading the entire index into memory each saving something we can do performancewise (as the cache is currently cleared)? Perhaps a solution would be to let the refresh pass back the modified information and not reread the file after each save (the PHP side could still update the file itself).
• Is it necessary to do a complete indexing each project load? Since we have a JSON index file with all the information in, we can just use that and go from there on startup. This can, however, cause problems if there are modifications to the files outside Atom itself (or while the package is deactivated). IDE's usually solve this by 'scanning for external changes' on startup. We could do something similar by storing the timestamp of the indexing in the JSON file and asynchronously walking through all the file names in the index on startup to see if any of the files were modified (and reindex them if so). We could also add some sort of 'Full rescan' command to reindex everything from scratch just in case. This saves a large indexing operation each start of a large project. This kind of scanning will sadly not work if the user installs new Composer dependencies as the only way we can pick them up will be through a new Composer class dump. We may be able to work around this by keeping an eye on the composer.lock file as it should be regenerated each composer update.

Sorry for the wall of text, but what do you think?

[Peekmo]

Hello,

I would prefer to not have any file at all ^^ As you said, it needs to maintain the json, and the other problem is that we will need to split the json file in a lot of little file (in order to avoid the loading of a HUGE json in memory).

Without file, everything is "live", no need to wait for an index writing and less disk I/O. Today, I think that the time for the PHP process is relatively low, so, I don't see any problem to keep things live.

That's my opinion, if the index process wasn't so bad (because of subprocess to be able to "handle" fatal errors), there will not any json file ^^

[ghost]

Ok, that makes sense. Is there any reason ClassMapRefresh is sometimes manually invoking PHP via buildIndexClass (i.e. can we perhaps use the ClassProvider directly as happens there as well)?

[Peekmo]

Yep, it's for updating the JSON file. ClassProvider just returns class info.

[ghost]

I may be missing something, but can't we just do something like:

protected function buildIndexClass($class)
{
    $provider = new ClassProvider();

    return $provider->execute(array($class, true));
}

instead of:

protected function buildIndexClass($class)
{
    $ret = exec(sprintf('%s %s %s --class %s',
        escapeshellarg(Config::get('php')),
        escapeshellarg(__DIR__ . '/../parser.php'),
        escapeshellarg(Config::get('projectPath')),
        escapeshellarg($class)
    ));

    // ... decode JSON and return.
}

Or is there a particular reason it happens in a separate process?

[Peekmo]

Because when there's a class with a fatal error in its code, it's impossible to catch the error and the script stopped. (It's possible to "recover" from a fatal error, but only once). So, to be able to continue the script after a fatal, I just exec a subprocess, so, the subprocess fails but not the parent one.

[ghost]

Ok, maybe it would be interesting to add that to the docblock of the method. I'll close this issue, since my OP no longer applies and now that I know what direction you want to go, I can more safely refactor without doing something that you don't want.