BreadcrumbRenderer performance
20 views
Skip to first unread message
j.r.
Feb 14, 2016, 10:51:54 AM2/14/16
to concordion-dev
Let's step into a new era of Concordion performance. :-)
The BreadcrumRenderer parses specifications another time to obtain the title element. Could we use another information source for the breadcrums.
What about using the file name of specification for breadcrums instead?
The BreadcrumRenderer parses specifications another time to obtain the title element. Could we use another information source for the breadcrums.
What about using the file name of specification for breadcrums instead?
Tim Wright
Feb 14, 2016, 12:05:25 PM2/14/16
to j.r., concordion-dev
I think we use the page title for the breadcrumb - which is quite nice.
However, we have the run results cache. Currently this does not store page title but would be trivial to do so.
--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/0be3edd6-0461-4268-b916-ea9e428e2926%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Nigel Charman
Feb 14, 2016, 2:11:08 PM2/14/16
to concord...@googlegroups.com
I'd be keen to keep the current
behaviour too. The logic is:
1) Use <title> text if available, else
2) Use the first <h1> heading text if available, else
3) Use the resource name
This is repeated for the "index page" resource in each folder recursively up the package structure.
Caching makes sense if we are running a suite, but doesn't help if we are running a single fixture. I already added a cache to BreadcrumbRenderer as part of the Concordion 2.0 changes. With this cache, we are still needing to parse the resource twice - once when the specification is run, and once when we get the breadcrumbs for it. Caching the breadcrumbs on the first parse would help - maybe by implementing DocumentParsingListener in BreadcrumbRenderer and storing the breadcrumb text of each index page as it is processed.
We could also look at other ways to get the <title> or <h1> text. For example, using a StaX parser or a crude text search.
@jacek - do you have performance figures for the Java implementation? It would be good to know whether it's worth tackling in Java or just .NET.
Nigel.
1) Use <title> text if available, else
2) Use the first <h1> heading text if available, else
3) Use the resource name
This is repeated for the "index page" resource in each folder recursively up the package structure.
Caching makes sense if we are running a suite, but doesn't help if we are running a single fixture. I already added a cache to BreadcrumbRenderer as part of the Concordion 2.0 changes. With this cache, we are still needing to parse the resource twice - once when the specification is run, and once when we get the breadcrumbs for it. Caching the breadcrumbs on the first parse would help - maybe by implementing DocumentParsingListener in BreadcrumbRenderer and storing the breadcrumb text of each index page as it is processed.
We could also look at other ways to get the <title> or <h1> text. For example, using a StaX parser or a crude text search.
@jacek - do you have performance figures for the Java implementation? It would be good to know whether it's worth tackling in Java or just .NET.
Nigel.
On 15/02/16 06:05, Tim Wright wrote:
j.r.
Feb 15, 2016, 2:19:50 PM2/15/16
to concordion-dev
Hi,
@nigel: I thought you can provide figures for the Java implementation, as you could tell that SimpleDateFormat takes 12ms in the PageFooterRenderer. B-)
For all the previous changes, I accepted your comments. Could we do it my way this time?
In my opinion the solution with the title element is not the best one.
For example, I recognized that the Concordion.NET test reports contain the bread crumbs "Concordion.NET > ...". When I clicked on the page I could not easily figure out, where it comes from: Neither did the page content contain such a headline nor was the root specification document named accordingly. I now recognized where it comes from. Perhaps, I am just not smart enough. :o)
Additionally, I observed that guys in our team often forget to change the title tag in our html specifications. But they provide decent names for the specification documents.
Moreover, some of the specifications don't contain a h1 headline. This is due to the fact that they are re-used / included in other pages.
Could we implement the simplest solution that could possibly work?
IMHO using the file names is good enough to solve a small thing such as bread crumbs. The requirements are simple and the code is simple.
Did I mention that I like simple solutions? :-)
@nigel: I thought you can provide figures for the Java implementation, as you could tell that SimpleDateFormat takes 12ms in the PageFooterRenderer. B-)
For all the previous changes, I accepted your comments. Could we do it my way this time?
In my opinion the solution with the title element is not the best one.
For example, I recognized that the Concordion.NET test reports contain the bread crumbs "Concordion.NET > ...". When I clicked on the page I could not easily figure out, where it comes from: Neither did the page content contain such a headline nor was the root specification document named accordingly. I now recognized where it comes from. Perhaps, I am just not smart enough. :o)
Additionally, I observed that guys in our team often forget to change the title tag in our html specifications. But they provide decent names for the specification documents.
Moreover, some of the specifications don't contain a h1 headline. This is due to the fact that they are re-used / included in other pages.
Could we implement the simplest solution that could possibly work?
IMHO using the file names is good enough to solve a small thing such as bread crumbs. The requirements are simple and the code is simple.
Did I mention that I like simple solutions? :-)
Nigel Charman
Feb 16, 2016, 5:00:06 AM2/16/16
to concord...@googlegroups.com
Fair call on the figures :)
Taking fairly crude measurements using the following code in BreadcrumbRenderer:
public void afterProcessingSpecification(SpecificationProcessingEvent event) {
try {
start = System.currentTimeMillis();
Element span = new Element("span").addStyleClass("breadcrumbs");
appendBreadcrumbsTo(span, event.getResource());
if (span.hasChildren()) {
getDocumentBody(event.getRootElement())
.prependChild(span);
}
long time = System.currentTimeMillis() -start;
System.err.println(" " + time + " : " + event.getResource().getName());
totalTime += time;
System.err.println(" " + totalTime + " : total");
} catch (Throwable t) {
t.printStackTrace();
}
}
Results are:
What sort of figures are you getting with the .NET implementation?
I'm reluctant to modify the current behaviour, since users will have built suites assuming the current behaviour and any change will impact on what they see in the breadcrumbs.
We could make the BreadcrumbRenderer an extension point and allow alternate implementations, possibly adding a configuration option to select the current implementation or filename only.
I'm keen for other opinions and suggestions.
Nigel.
Taking fairly crude measurements using the following code in BreadcrumbRenderer:
public void afterProcessingSpecification(SpecificationProcessingEvent event) {
try {
start = System.currentTimeMillis();
Element span = new Element("span").addStyleClass("breadcrumbs");
appendBreadcrumbsTo(span, event.getResource());
if (span.hasChildren()) {
getDocumentBody(event.getRootElement())
.prependChild(span);
}
long time = System.currentTimeMillis() -start;
System.err.println(" " + time + " : " + event.getResource().getName());
totalTime += time;
System.err.println(" " + totalTime + " : total");
} catch (Throwable t) {
t.printStackTrace();
}
}
Results are:
- For a single test, between 20-80ms out of a total test time around 500-800ms.
- For the full Concordion suite, a total time of about 360 ms out of a 6 sec test run.
- single test, 5-10 ms
- full suite, 240 ms
What sort of figures are you getting with the .NET implementation?
I'm reluctant to modify the current behaviour, since users will have built suites assuming the current behaviour and any change will impact on what they see in the breadcrumbs.
We could make the BreadcrumbRenderer an extension point and allow alternate implementations, possibly adding a configuration option to select the current implementation or filename only.
I'm keen for other opinions and suggestions.
Nigel.
--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/88f9a762-c21a-4479-8f29-fe0858285d09%40googlegroups.com.
j.r.
Feb 16, 2016, 11:07:23 AM2/16/16
to concordion-dev
The profiler on the cross-compiled Concordion classes provides the following performance data on the .NET platform:
- For the full Concordion.NET suite, BreadcrumbRenderer.createBreadcrumbElements takes 309ms out of 2522ms for the entire run.
As a result, we could gain more than 10% by improving this code.
It would be cool, if the cross-compiled version would be faster than the original C# implementation based on this optimization. :-)
After your explanation, I understand your concerns about the changed behavior existing users.
What about asking them, if the change would be o.k.?
Michael Robinson
Feb 17, 2016, 9:45:41 PM2/17/16
to j.r., concordion-dev
On Tue, Feb 16, 2016 at 08:07:23AM -0800, j.r. wrote:
> After your explanation, I understand your concerns about the changed
> behavior existing users.
>
> What about asking them, if the change would be o.k.?
As I understand it, you're asking if we can change the breadcrumb renderer
> After your explanation, I understand your concerns about the changed
> behavior existing users.
>
> What about asking them, if the change would be o.k.?
from the title tag to the filename?
We stopped using the breadcrumb renderer, after we observed two things:
1. The breadcrumbs are rarely helpful to determine structure; we generate
indexes based off annotations, h1 information, and other things instead;
and
2. Nobody ever gets the title tag right in the first place; our analysts
would typically cut'n'paste from a different specification, and not bother
to update the title (as it was only shown in the browser's title bar).
Michael.
Tim Wright
Feb 17, 2016, 9:56:36 PM2/17/16
to Michael Robinson, j.r., concordion-dev
--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send an email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/20160218024533.GA14524%40diaspora.gen.nz.
For more options, visit https://groups.google.com/d/optout.
Nigel Charman
Feb 18, 2016, 4:15:44 AM2/18/16
to concord...@googlegroups.com
Sure does!
One other factor that comes into play is the different specification types. For Markdown and Excel specs, I don't think we ever set the <title>, and we could improve the performance of obtaining breadcrumb headings for these by not creating an XOM document.
So we may need to allow configuring the breadcrumb renderer for all specification types, or for a particular type.
Grrr, I was going to leave this until after the 2.0 release, but since it could change the new specification type interface, let's look at it for 2.0 now.
I'm still keen that we keep the same default behaviour for backwards compatibility (and since we won't be able to ask all users what they think!). Any objection to this?
One other factor that comes into play is the different specification types. For Markdown and Excel specs, I don't think we ever set the <title>, and we could improve the performance of obtaining breadcrumb headings for these by not creating an XOM document.
So we may need to allow configuring the breadcrumb renderer for all specification types, or for a particular type.
Grrr, I was going to leave this until after the 2.0 release, but since it could change the new specification type interface, let's look at it for 2.0 now.
I'm still keen that we keep the same default behaviour for backwards compatibility (and since we won't be able to ask all users what they think!). Any objection to this?
To post to this group, send email to concord...@googlegroups.com.
Visit this group at https://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/CAJifTypk-syGJYhYij2eBA8OhGGj6fnG13Dvj_8wS2%3D35WJuqw%40mail.gmail.com.
Nigel Charman
Feb 18, 2016, 4:18:52 AM2/18/16
to concord...@googlegroups.com
Michael,
I'm assuming this means you're using your own runner that overrides the
default ConcordionBuilder behaviour?
If so, is there anything else in ConcordionBuilder that you think it
would be useful to be able to override?
Nigel.
I'm assuming this means you're using your own runner that overrides the
default ConcordionBuilder behaviour?
If so, is there anything else in ConcordionBuilder that you think it
would be useful to be able to override?
Nigel.
Michael Robinson
Feb 18, 2016, 8:34:22 PM2/18/16
to Nigel Charman, concord...@googlegroups.com
On Thu, Feb 18, 2016 at 10:18:46PM +1300, Nigel Charman wrote:
> I'm assuming this means you're using your own runner that overrides
> the default ConcordionBuilder behaviour?
Interesting. I just had cause to go and read the BreadcrumbRenderer
> I'm assuming this means you're using your own runner that overrides
> the default ConcordionBuilder behaviour?
source, and have to say it'd be useful to have better documentation
about it. It assumes that, for example, you'll have network/Network.html
somewhere in the chain, so it can render a breadcrumb to it.
On the teams I've been working on, we've never bothered to set things
up that way, so the breadcrumbs don't render.
So it'd be more accurate to say it's there, but it doesn't render.
> If so, is there anything else in ConcordionBuilder that you think it
> would be useful to be able to override?
read of them.
One thing that springs to mind is the restriction on using the Concordion
namespace for commands. Now we're in the exciting new world of Markdown,
what was previously irritating is now a problem; unless there's some
way of getting a different prefix into the Markdown command interpreter?
I'll see if I can send through a patch and some motivating documentation.
Michael.
Nigel Charman
Feb 19, 2016, 12:56:32 PM2/19/16
to Michael Robinson, concord...@googlegroups.com
Hey Michael
On 19/02/16 14:34, Michael Robinson wrote:
Interesting. I just had cause to go and read the BreadcrumbRenderer source, and have to say it'd be useful to have better documentation about it.
The specs for this are at
http://concordion.github.io/concordion/latest/spec/results/breadcrumbs/Breadcrumbs.html.
Is this the type of documentation you're looking for?
While this is mentioned on the Tutorial page of the website, we're putting it into a new "Documentation" page on the new site to make it easier to find.
While this is mentioned on the Tutorial page of the website, we're putting it into a new "Documentation" page on the new site to make it easier to find.
It assumes that, for example, you'll have network/Network.html somewhere in the chain, so it can render a breadcrumb to it.
I'm not sure where you're seeing this? I can't see it on the master
branch, and am not aware of it being there in a previous version.
With the Markdown parser now in core, we added a ConcordionOptions annotation which allows for additional options such as this to be configured.
This is in the TODO list, and there's a placeholder for it at the bottom of the MarkdownGrammar spec (http://concordion.github.io/concordion/latest/spec/specificationType/markdown/MarkdownGrammar.html)..... One thing that springs to mind is the restriction on using the Concordion namespace for commands. Now we're in the exciting new world of Markdown, what was previously irritating is now a problem; unless there's some way of getting a different prefix into the Markdown command interpreter?
With the Markdown parser now in core, we added a ConcordionOptions annotation which allows for additional options such as this to be configured.
I'll see if I can send through a patch and some motivating documentation.
That would be great!
If anyone wants to pick up the change to override the default behaviour of BreadcrumbRenderer in ConcordionBuilder, I'd be happy for a PR on that too :)
If anyone wants to pick up the change to override the default behaviour of BreadcrumbRenderer in ConcordionBuilder, I'd be happy for a PR on that too :)
j.r.
Feb 25, 2016, 2:10:18 PM2/25/16
to concordion-dev
created pull request for dynamic replacement of the implementation of BreadcrumbsRenderer :-)
https://github.com/concordion/concordion/pull/176
https://github.com/concordion/concordion/pull/176
Nigel Charman
Mar 5, 2016, 8:27:10 PM3/5/16
to concordion-dev
Hi Michael
I've added namespace support for Markdown specs to master now. See https://github.com/concordion/concordion/pull/182.
To enable this, you need to annotate your fixture class with:
@ConcordionOptions(declareNamespaces={"ext", "urn:concordion-extensions:2010"}).
where the value of declareNamespaces is a list of strings, where the values alternate between namespace prefixes and the namespace they are mapped to.
The Markdown file can then declare commands in the new namespace:
[-](- "ext:embed=getDetails()")
I'm planning to release an updated SNAPSHOT including this change in the next few days - this is likely to be a release candidate since 2.0 is feature complete.
Nigel.
I've added namespace support for Markdown specs to master now. See https://github.com/concordion/concordion/pull/182.
To enable this, you need to annotate your fixture class with:
@ConcordionOptions(declareNamespaces={"ext", "urn:concordion-extensions:2010"}).
where the value of declareNamespaces is a list of strings, where the values alternate between namespace prefixes and the namespace they are mapped to.
The Markdown file can then declare commands in the new namespace:
[-](- "ext:embed=getDetails()")
I'm planning to release an updated SNAPSHOT including this change in the next few days - this is likely to be a release candidate since 2.0 is feature complete.
Nigel.
Reply all
Reply to author
Forward
0 new messages