On Tue, Dec 20, 2011 at 06:14:54PM -0500, Dan Espen wrote:
> After removing the ones we don't need (above), there's not many calls left.
> We only deal with a few directories. We could readdir and build a hash.
Yes, but the recusiveness of those subroutines before the hashing I added in
the patch were having to continally stat the same directories when they were
told to recurse. Using File::Find would avoid this -- and since it's a core
module anyway, would save us also reinventing the wheel and maintaining it.
Refactoring to improve readability, BTW, even if the gain is only negligable
-- or where the techniques used are an improvement within the remit of the
language -- is something I tend to like doing. In this case, using more
core perl modules is always an easy win.
> Regarding interpret menu, it deals with trees build by the parser.
> I'm wondering if we should try to use the 'sub' callback method
> instead of 'tree' for the xml parse.
That's one possibility. But my main gripe with this is not so much how the
XML is parsed but the way in which the information is stored (I agree with
you, we're storing redundant data) and then accessed.
We've got a multitude of hashes split off from larger hashes, banded about
between different subroutines -- the way the data is stored does not make it
at all clear what the end result of the parsing of this data is for -- and
that's my critical problem; maintaining this becomes impossible.
Take a look at interpret_entry_node() -- considering that splice() is
already used elsewhere in the code, for instance, it should also have been
used here -- I shudder when I see this:
elsif ($tree->[$i] eq '0')
{
$i++;
$i++;
}
It's just sloppy -- and shows there was little thought put in to what the
data representation of this should be -- and arrays clearly weren't meant to
be it here -- not when you have to increment $i by 2 in some cases.
It's not as if this is useful -- you'd have to explicitly remember the
ordering of the items in the array; make some guarantee that they're always
returned in the same order, and perhaps even go to lengths to ensure that.
Not to mention we're looking at O(n) for the lookups, maybe a little less if
the array is splice()d, but it still sucks. If there's more than one node
we're looking over (which we are in the case of this tree structure), then
that's O(n * m) -- just because the wrong structure was used.
And no, I am not interested in big-oh really, I'm just using it to prove a
point that even in something as simple as this -- the stat() call problem is
masking another more fundamental problem which NYTProf is hinting at
slightly for us.
But I know you are not responsible for this Dan, it's just academically
interesting to note these observations.
> Putting the tree in memory and then scanning it
> seems like extra work. Especially because half the entries appear to
> be something we don't want.
> I don't think it would be an easy change though.
You're right, it wouldn't be an easy change, which is why I am very hesitant
at Thomas Funk's improvements to this -- ripping out the logic for some of
these interpret_*() subroutines is one thing -- but that's pretty pointless
when the turd you're polishing is already leaving the smear stains behind it
as you do so.
So if were going to do this, this is where I would start -- looking at the
structure of what XML::Parse is giving us, and what data we need from XDG --
when we can reliably get a sense of that, then we can look at changing
structures, etc.
This is something I tried to get across in my reply to the last thread on
fvwm-menu-desktop; perhaps this reply is clearer. I do hope so.
-- Thomas Adam
--
"Deep in my heart I wish I was wrong. But deep in my heart I know I am
not." -- Morrissey ("Girl Least Likely To" -- off of Viva Hate.)
