3
\$\begingroup\$

I have below snippet of code which gets all the available menu items for a zone(top or left)and adds the widgets for which user has access to an ArrayList collection. For now the getWidgetsByZone(zone) returns 64 items and iterates over them. Am seeing some performance lag in this method(in a tool called GlowRoot which logs the time trace for each user action) I dont know why. Can someone help me to optimize performance for my scenario?

AM using JDK 7, Hibernate 3.6 and Spring 3.1

The method performance trace in GlowRoot is as below

60.0% com.dc.core.presentation.presenter.impl.WebContentPresenter.getMenuHTML(WebContentPresenter.java:435)
50.0% com.dc.core.presentation.service.impl.DashboardService.getMenuItems(DashboardService.java:258)
30.0% com.dc.core.presentation.service.impl.DashboardService.isAccessible(DashboardService.java:382)

Here is my WebContentPresenter.getMenuHTML() implementation

public String getMenuHTML(String baseUrl, String zone, String cssClass, IPersistentEntityInstance entityInstance) {
    (line 435) List<IContentWidget> instances = dashboardService.getMenuItems(zone);
    StringBuffer html = new StringBuffer();

    if (instances == null || instances.isEmpty()) {
        html.append("&nbsp;");
    }
    else {
        Map<Long, List<IContentWidget>> treeData = new HashMap<Long, List<IContentWidget>>();
        for (IContentWidget instance : instances) {
            BeanWrapperImpl bean = new BeanWrapperImpl(instance);
            Object parent = bean.getPropertyValue("parent");
            Long parentId = -1L;
            if (passportContextService.getIsInContext(instance)) {
                if (parent != null) {
                    parentId = ((IContentWidget) parent).getId();
                }
                List<IContentWidget> children = treeData.get(parentId);
                if (children == null) {
                    children = new ArrayList<IContentWidget>();
                }
                children.add(instance);
                treeData.put(parentId, children);
            }
        }

        generateTreeHtml(html, treeData, -1L, baseUrl, "parent", entityInstance,
                authorizationService.userHasAdminPermission(SecurityHelper.getAuthenticatedUser()));
    }
    return String.format("<ul class=\"%s\">%s</ul>", cssClass, html.toString());
}

Here is DashboardService.getMenuItems() implementation

public List<IContentWidget> getMenuItems(String zone) {
        List<IContentWidget> widgets = new ArrayList<IContentWidget>();
        if (zone != null) {

            List<IPersistentEntityInstance> instances = getWidgetsByZone(zone);

            for (IPersistentEntityInstance instance : instances) {
                IContentWidget contentWidget = (IContentWidget) instance;
                if (contentWidget.getZones() == null) continue;

                // block widgets that should only show up in mobile / responsive ui
                if (contentWidget.getZones().contains(RESPONSIVE_VISIBLE)) continue;

                // Add widget only if the current user has read permission on the entity.
                if (contentWidget.getTargetItemScreen() != null || contentWidget.getTargetListScreen() != null) {
                    (line 258 )if (isAccessible(contentWidget)) {
                        widgets.add(contentWidget);
                    }
                }
                else {
                    widgets.add(contentWidget);
                }
            }
        }
        Collections.sort(widgets, new Comparator<IContentWidget>() {

            public int compare(IContentWidget o1, IContentWidget o2) {
                int i = o1.getOrderNum() - o2.getOrderNum();
                return i == 0 ? 0 : i < 0 ? -1 : 1;
            }

        });
        return widgets;
    }

Implementation of DashboardService.isAccesible()

private boolean isAccessible(IContentWidget contentWidget) {
    boolean isWidgetAccessible = false;
    String permission = contentWidget.getDisplayPermission();
    if (permission != null) {
        isWidgetAccessible = authorizationService.userHasPermission(SecurityHelper.getAuthenticatedUser(),
                permission);
    }
    else {
        IBaseScreen screen = contentWidget.getTargetItemScreen() == null ? contentWidget.getTargetListScreen()
                : contentWidget.getTargetItemScreen();
        // return true when target screen is 'null', this means that target link cannot be secured because it is not
        // associated with any entity
        if (screen == null) {
            isWidgetAccessible = true;
        }
        else {
            (line 382)IAccessEntry access = authorizationService.getAccessForEntityMetadata(screen.getEntityMetadata());

            // fetching metadata from entityMetadataService again to trigger population of facade
            if (screen instanceof IListScreen && access.getIsReadable()) {
                isWidgetAccessible = true;
            }
            else if (screen instanceof IItemScreen && access.getIsCreatable()) {
                isWidgetAccessible = true;
            }
        }
    }
    return isWidgetAccessible;
}

Implementation of getWidgetsByZone method

public List<IPersistentEntityInstance> getWidgetsByZone(String zone) {
    IEntityMetadata entity =  entityService.findEntityMetadataByName(ContentWidget.class.getSimpleName());
return entityService.runNamedQuery(entity, NamedQueryList.WIDGETS_BY_ZONE, new Object[] { zone });
    }

Here is my ContentWidget Entity

@LocalOnly
@Entity
@EntityMetadataDefaults(editable = false)
@Audited
@NamedQueries({
    @NamedQuery(name = NamedQueryList.DASHBOARD_WIDGETS, query = "from ContentWidget where zones like '%dashboard%' and dashboardContexts.size = 0 order by orderNum", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }),
    @NamedQuery(name = NamedQueryList.WIDGETS_BY_ZONE, query = "from ContentWidget where zones like '%' || ? || '%' order by orderNum", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }),
    @NamedQuery(name = NamedQueryList.WIDGETS_BY_ZONE_ORDER_BY_NAME, query = "from ContentWidget where zones like '%' || ? || '%' order by displayName", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }),
    @NamedQuery(name = NamedQueryList.WIDGETS_BY_DASHBOARD_URL, query = "from ContentWidget where dashboardUrl like ? || '%'", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }),
    @NamedQuery(name = NamedQueryList.WIDGETS_BY_NAME, query = "from ContentWidget where name = ?", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }),
    @NamedQuery(name = NamedQueryList.WIDGETS_BY_CONTEXT, query = "from ContentWidget where zones like '%dashboard%' and ? in elements(dashboardContexts) order by orderNum", hints = {
            @QueryHint(name = "org.hibernate.cacheable", value = "true"),
            @QueryHint(name = "org.hibernate.cacheRegion", value = "Metadata") }) })
@Cacheable
@Cache(usage = CacheConcurrencyStrategy.READ_WRITE, region = "Metadata")
@EventDefaults({
        @EventHandlerDefaults(beanName = "contentWidgetPermissionCommand", eventType = EventTypeEnum.OnBeforeCreate),
        @EventHandlerDefaults(beanName = "contentWidgetPermissionCommand", eventType = EventTypeEnum.OnBeforeUpdate) })
@ReadPermission(name = AuthorizationService.ANONYMOUS_PERMISSION)
public class ContentWidget implements IContentWidget {
    private static final long       serialVersionUID = 1680304771254400928L;
    private String                  packageGuid;
    private Long                    id;

    private String                  name;                                   // unique name to reconcile imported data
    private String                  displayName;                            // used by UI
    private String                  description;                            // anchor title attribute
    private int                     orderNum;                               // universal ordering
    private String                  zones;                                  // csv: "top,left,dashboard,context,..."
    private String                  iconClass;
    private String                  displayPermission;

    // menu settings
    private IContentWidget          parent;

    private String                  targetUrl;

    private IListScreen             targetListScreen;
    private IItemScreen             targetItemScreen;
    private boolean                 isCacheable;
    private boolean                 isDivider;

    private boolean                 isPopup;

    private List<IEntityMetadata>   contextEntities;                        // for contextual menus
    protected IFilterDefinition     entityFilterDefinition;
    // dashboard settings
    private int                     dashboardWidth   = 1;
    private String                  dashboardUrl;
    private String                  dashboardWidgetType;
    private IListScreen             dashboardListScreen;
    private IItemScreen             dashboardItemScreen;
    private List<IEntityMetadata>   dashboardContexts;                      // for item screen dashboards

    private ISessionService         sessionService;
    @Autowired
    private IPassportContextService passportContextService;
    @Autowired
    private IReportingConfiguration reportingConfiguration;
    private Timestamp               createdAt;
    private Timestamp               updatedAt;
    private ICustomNamedQuery       menuCountQuery;
    private Set<IPassportContext>   passportContexts;
    }
\$\endgroup\$
  • \$\begingroup\$ What's this about in the middle of your code: (line 435) ? There are others too, for example (line 382). Please scan through your post and remove such unnecessary elements. \$\endgroup\$ – janos Sep 23 '14 at 20:22
1
\$\begingroup\$

It's not easy to see how to improve the performance significantly. But I do see a couple of minor issues that can be improved, and which may bring some (though perhaps only marginal) performance improvements too.


List<IContentWidget> instances = dashboardService.getMenuItems(zone);

if (instances == null || instances.isEmpty()) {
    html.append("&nbsp;");
}

Your getMenuItems method never returns null, so you can drop the null check.


Map<Long, List<IContentWidget>> treeData = new HashMap<Long, List<IContentWidget>>();

Since you're in Java 7, you can simplify to:

Map<Long, List<IContentWidget>> treeData = new HashMap<>();

Long parentId = -1L;
if (passportContextService.getIsInContext(instance)) {
    if (parent != null) {
        parentId = ((IContentWidget) parent).getId();
    }
    List<IContentWidget> children = treeData.get(parentId);
    if (children == null) {
        children = new ArrayList<IContentWidget>();
    }
    children.add(instance);
    treeData.put(parentId, children);

A few things to note here:

  • Does parentId really need to be Long as opposed to a long?
  • You should move the declaration inside the if, because you're not using it outside.
  • What do you expect to happen if parent == null? Will treeData.get(-1) make sense? Because if not then you might need to move that inside the if (parent != null) { ... } block
  • Since you're in Java 7, instead of new ArrayList<IContentWidget>(), you should write new ArrayList<>(). Also consider if it might make sense to use an immutable empty list instead, Collections.emptyList(). (I don't know, just a thought.)

This is outright silly:

public int compare(IContentWidget o1, IContentWidget o2) {
    int i = o1.getOrderNum() - o2.getOrderNum();
    return i == 0 ? 0 : i < 0 ? -1 : 1;
}

You could write simply as:

public int compare(IContentWidget o1, IContentWidget o2) {
    return o1.getOrderNum() - o2.getOrderNum();
}

However, keep in mind that both alternatives may break due to integer overflow, for example if o1.getOrderNum() is a large negative number and o2.getOrderNum() is a large positive number.

Actually, the best is as @mjolka suggested in a comment:

return Integer.compare(o1.getOrderNum(), o2.getOrderNum());

public List<IContentWidget> getMenuItems(String zone) {
    List<IContentWidget> widgets = new ArrayList<IContentWidget>();
    if (zone != null) {
        // ...
    }
    Collections.sort(widgets, new Comparator<IContentWidget>() {
        // ...
    });
    return widgets;
}

Here, if zone == null, you'll still do a bunch of unnecessary things:

  • Create an empty list
  • Sort the empty list

You should reorganize a bit, for example:

public List<IContentWidget> getMenuItems(String zone) {
    if (zone == null) {
        return Collections.emptyList();
    }
    List<IContentWidget> widgets = new ArrayList<IContentWidget>();
    // ...
    Collections.sort(widgets, new Comparator<IContentWidget>() {
        // ...
    });
    return widgets;
}
\$\endgroup\$
  • \$\begingroup\$ How about Integer.compare(o1.getOrderNum(), o2.getOrderNum());? \$\endgroup\$ – mjolka Sep 24 '14 at 0:39
  • \$\begingroup\$ Good point, added to my post, thanks for pointing out! \$\endgroup\$ – janos Sep 24 '14 at 21:13
1
\$\begingroup\$
        if (screen instanceof IListScreen && access.getIsReadable()) {
            isWidgetAccessible = true;
        }
        else if (screen instanceof IItemScreen && access.getIsCreatable()) {
            isWidgetAccessible = true;
        }

These could get merged.

if ( (screen instanceof IListScreen && access.getIsReadable()) 
  || (screen instanceof IItemScreen && access.getIsCreatable())
   ) {
    isWidgetAccessible = true;
}

rolfl points out that you can go one step further.

Since isWidgetAccessible can only have two values (true and false), and it's already AT false, you can say

isWidgetAccessible = (screen instanceof IListScreen && access.getIsReadable()) || (screen instanceof IItemScreen && access.getIsCreatable());

Because if it's that false, you're just setting false to a variable who's value was already false to begin with.

\$\endgroup\$
  • \$\begingroup\$ I would take this further to just: isWidgetAccessible = (screen instanceof IListScreen && access.getIsReadable()) || (screen instanceof IItemScreen && access.getIsCreatable()); \$\endgroup\$ – rolfl Oct 30 '14 at 13:55

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.