Use setters instead of public properties
ClosedPublic
Actions

Authored by joshuaspence on Nov 17 2015, 9:04 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rPHU67f26422b91a: Use setters instead of public properties

Summary

Currently a bunch of properties in the AASTNode class are public instead of using explicit setter methods. This doesn't seem to have any effect on performance as far as I can tell

Test Plan

Ran the following script with 100 iterators:

P1892 xhpast_tree.php

1	<?php
2
3	require_once __DIR__.'/scripts/__init_script__.php';
4
5	$console = PhutilConsole::getConsole();
6	$iterations = 10;
7	$source = Filesystem::readFile(__DIR__.'/../phabricator/src/__phutil_library_map__.php');
8	$start = microtime(true);
9
10	for ($ii = 0; $ii < $iterations; $ii++) {
11	XHPASTTree::newFromData($source);
12	}
13
14	$end = microtime(true);
15	$duration = $end - $start;
16
17	$console->writeOut("Average: %fs\n", $duration / $iterations);
18	$console->writeOut("Total: %fs\n", $duration);

Before

Average: 2.679129s
Total:   267.912916s

After

Average: 2.648229s
Total:   264.822857s

Diff Detail

Repository

rPHU libphutil

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

joshuaspence updated this revision to Diff 35089.Nov 17 2015, 9:04 PM

joshuaspence retitled this revision from to Use setters instead of public properties.

joshuaspence updated this object.

joshuaspence edited the test plan for this revision. (Show Details)

joshuaspence added a reviewer: epriestley.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptNov 17 2015, 9:04 PM

Herald added a subscriber: Korvin. · View Herald Transcript

Maybe double check with PhabricatorApplicationTransactionEditor.php? __phutil_library_map__.php has a lot of lines, but the tree isn't actaully that complex.

This revision is now accepted and ready to land.Nov 17 2015, 9:06 PM

joshuaspence added inline comments.Nov 17 2015, 9:10 PM

src/parser/aast/api/AASTNode.php
92	Hmm, this seems wrong/odd... adding an `assert_instances_of` call here doesn't seem to impact the execution time; Average: 2.559327s Total: 255.932677