-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve performance of indent-region
.
#47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @taku0. Thanks for the PR. 5 minutes is untenable and 6 seconds seems pretty good. This is definitely an area for improvement for jsonian.
I'm going to keep playing with your code, and I'll do a more through review by the end of the weekend.
From playing with your fork, I see, in addition to the vast speed improvement, an inconsistency.
Given the following (modified) end segment of `large-json-file.json:
"type": "object",
"required": [
"errors",
"id",
"location",
"msiArmId",
$ "name",^
"provisioningState",
"storageAccountArmId",
"systemData",
"type"
]
}
}
}
}
Running jsonian-indent-line
with the cursor at $
correctly moves "name",
back into line. However, running indent-region
with the point at $
and the mark at ^
indents "provisioningState",
to match "name",
:
"type": "object",
"required": [
"errors",
"id",
"location",
"msiArmId",
$ "name",^
"provisioningState",
"storageAccountArmId",
"systemData",
"type"
]
}
}
}
}
This is obviously not correct. Can you take a look?
(jsonian--forward-to-significant-char) | ||
(current-column)))) | ||
(- (max start end) (min start end))))) | ||
(forward-line 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be willing to explain why your implementation of jsonian--infer-indentation
is faster and the intuition behind how it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed jsonian--infer-indentation
not for performance but for robustness. For example, existing implementation doesn't behave well for the following examples (where ■ is the point):
{
"a":
1
■
}
[
[
[
■1
]
]
]
The new implementation travels from the innermost parent to the outermost ancestor, inferring the indentation from their first child with conditions:
- The open bracket must be at the end of the line (excluding spaces).
- The array/object must not be empty.
- The first child must be before the point.
- The inferred indentation must be greater than zero.
If we cannot infer an indentation, inferring from the outermost ancestor without the third condition.
(let ((indent nil) | ||
(origin (point)) | ||
(done nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be consistent with how we declare variables. parent-position
is declared (defaulted to nil
), we should do the same with indent
and done
. This is constant with the rest of the code base.
(let ((indent nil) | |
(origin (point)) | |
(done nil) | |
(let (indent | |
(origin (point)) | |
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention is that if a variable isn't initialized at declaration but needs to be initialized later, I declare it as a bare identifier. Otherwise, i.e. if a variable is initialized to nil-as-false or nil-as-a-not-found-marker, I explicitly initialize it to nil. This is just a quirk of mine growing up as a Java programmer. I will follow whatever styles you prefer.
Add `jsonian-indent-region' and rewrite `jsonian-indent-line' to speed up `indent-region'.
It's an off-by-one error. Fixed. |
Maybe I have to adapt your terminology. The following is my understanding. Is it correct?
Example: [
[
[
"abc"
]
]
] At string "abc":
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay. I dropped the ball on review.
I have read through the code a couple more times, to the point where I have an at least cursory understanding. As far as I can tell, this is currently an improvement to both accuracy and speed, so I'm going to merge as is.
I'll find some time later to do a cleanup pass to unify naming and comments where my understanding is murky.
Thanks for the PR!
Add
jsonian-indent-region
and rewritejsonian-indent-line
to speed upindent-region
.When indenting whole
large-json-file.json
,jsonian-indent-region
takes only 6 seconds whileindent-region-line-by-line
, the default backend forindent-region
, takes 5 minutes.Background:
I'm author of JSON Par, minor mode for structural editing of JSON, which is based on
json-mode
. As you know,json-mode
is abandoned and I couldn't contact the author, I would like to migrate fromjson-mode
to yourjsonian.el
. JSON Par overridesindent-region-function
and other basic functions to improve performance but it should be in the base mode.