Skip to content

Conversation

@ajayk
Copy link
Contributor

@ajayk ajayk commented Sep 16, 2015

No description provided.

@johnjaylward
Copy link
Contributor

    /**
     * Get the number of elements in the JSONArray, included nulls.
     *
     * @return The length (or size).
     */
    public int length() {
        return this.myArrayList.size();
    }

given this, I can't see this change offering much except it increases memory be 1 int length and saves 2 derefrences.

unless you are iterating over millions of items, I don't see this impacting performance at all

@stleary
Copy link
Owner

stleary commented Sep 16, 2015

This is a proposed performance improvement, not a bug fix or enhancement, and contains no behavior or API changes. This lowers the acceptance bar a bit. I have not looked at performance at all yet; I would think a number of potential improvements could be made.

  • It would be helpful to see some kind of quantification of before/after results to justify making any performance change.
  • CDL.java has a number of for loops. Some get the length before the loop, and others call length() within the loop. Any reason why you are not proposing to change all of them to make them consistent?
  • Have you profiled the code to determine where are the worst performance bottlenecks? Maybe some other code changes would yield a bigger improvement.

Here is a discussion that might provide some insight into the change you are proposing:
http://stackoverflow.com/questions/6093537/for-loop-optimization

@ajayk
Copy link
Contributor Author

ajayk commented Sep 16, 2015

@stleary @johnjaylward Thanks for the feedback , i will get back u with another pull request ,with the changes, more concrete benchmarks quantification

@stleary
Copy link
Owner

stleary commented Oct 14, 2015

No further action at this time.

@stleary stleary closed this Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants